Skip to content
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

feat: support multiple database connections #56

Merged
merged 19 commits into from
Feb 20, 2022
Merged

feat: support multiple database connections #56

merged 19 commits into from
Feb 20, 2022

Conversation

tsangste
Copy link
Contributor

@tsangste tsangste commented Feb 19, 2022

The resolves #20

Changes:

  • Create injection tokens for MikroORM and EntityManager
  • Add contextName param for getRepositoryToken and InjectRepository
  • Create new forMiddleware() register for MikroOrmModule so we can register multiple MikroORM's for multiple databases

@B4nan
Copy link
Member

B4nan commented Feb 19, 2022

Thanks, will try to take a look during next few days.

Worth saying that I don't think we need any breaking changes to support this. This should all be optional and only a new section in the docs should be added, rest should stay unchanged (I mean the examples mainly, and extra decorator usage).

Disabling the automatic request context middleware would be a huge breaking change I am not willing to make, but I really believe we can find a way to keep things as they are and just add the new feature. Maybe we could just base this on the registerRequestContext option, so with multiple databases you would disable it and call the forMiddleware explicitly, while keeping full back compatibility with a single forRoot call.

@tsangste
Copy link
Contributor Author

tsangste commented Feb 20, 2022

That sounds good I'll have another tackle at it with your feedback to minimise breaking changes

@tsangste
Copy link
Contributor Author

@B4nan updated code without breaking changes

README.md Show resolved Hide resolved
@tsangste tsangste requested a review from B4nan February 20, 2022 11:42
Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good so far, found one nit, also the build fails with some TS errors

src/mikro-orm.common.ts Outdated Show resolved Hide resolved
tsangste and others added 2 commits February 20, 2022 12:04
Co-authored-by: Martin Adámek <banan23@gmail.com>
@tsangste
Copy link
Contributor Author

tsangste commented Feb 20, 2022

Yeah when making some more changes to the injection tokens it broken some stuff but I have resolved them now

@tsangste tsangste requested a review from B4nan February 20, 2022 12:06
Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good, one final nitpick

src/mikro-orm-core.module.ts Outdated Show resolved Hide resolved
@B4nan B4nan changed the title Multiple database connections feat: support multiple database connections Feb 20, 2022
@B4nan B4nan merged commit df4725b into mikro-orm:master Feb 20, 2022
@tsangste tsangste deleted the multiple_databases branch February 20, 2022 13:37

export const getEntityManagerToken = (name: string) => `${name}_EntityManager`;
export const InjectEntityManager = (name: string) => Inject(getEntityManagerToken(name));
export const getSqlEntityManagerToken = (name: string) => `${name}_SqlEntityManager`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doing some refactors and came to this place. do we actually need those? if there will be more EMs, we will pick based on the context name, not on their subtype

will try to remove it, as I believe it shouldn't be needed, one decorator should be enough - users will pick the right subtype on the callsite, there is no inference involved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I wasn't sure if we needed it or not but it does make sense we can use the single EM Injection Token for all the types

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.

Multiple database connections
2 participants