Skip to content

feat: allow use of scoped containers #21

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: allow use of scoped containers #21

wants to merge 2 commits into from

Conversation

ashleyw
Copy link
Contributor

@ashleyw ashleyw commented May 13, 2019

Hi,

By default, TypeORM only supports a global container[0], which is referenced whenever you call getManager().

This PR contains my potential solution but requires your services have a public container property exposed so the @Transactional decorator can access it, and an active ConnectionManager instance in the container.

Is there a better way of doing this? Is this feature something you'd be interested in merging? Please let me know your thoughts -- I'm sure there must be a cleaner way of achieving this!

Thanks!

[0] https://github.com/typeorm/typeorm/blob/master/src/container.ts

@odavid
Copy link
Owner

odavid commented May 17, 2019

Hi @ashleyw - Thank you!

I am sorry I could not respond earlier, and I am sorry in advance if it will take me some time to do so in the future.

The part that we need a container property within the owner service feels a bit "hacky" to me.

I haven't give it a lot of thinking, but maybe instead of using a "convention" and adding the container property to the owner service, there could be a Factory injected to the cls-hooked context during the initialization passing a function to the initializeTransactionalContext function.

Not sure if the above can solve the problem, but it would feel much better if the "contract" will not rely on member name.

Can you explain a bit the use case of scoped containers for creating connections?

Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants