mikro-orm
fix: resolve issue with ordering of middleware being applied for multiple database
When importing MikroOrmModule.forMiddleware() to enable request context for multiple databases there is a chance it can throw Using global EntityManager instance methods for context specific actions is disallowed. when interacting with the EM within another middleware. This is due to the forMiddleware() doing a child import which causes the request context middleware to be registered at a later time.
changes:
- rename
mikro-orm-middlewaretomultiple-mikro-ormand allowforRootregistering - exposed
multiple-mikro-ormvia barrel - create
MULTPLE_MIKRO_ORM_MODULE_OPTIONStoken for multiple-mikro-orm - remove multiple register comment as that is not the default standard way of doing it
- preserve previous
MikroOrmModule.forMiddleware()for backwards compatibility (to be removed later)
i cant say i like the naming, the word multiple feels weird here, also the whole point of that module is to deal with the middleware, why should we rename it in the first place?
i cant say i like the naming, the word
multiplefeels weird here, also the whole point of that module is to deal with the middleware, why should we rename it in the first place?
The module doesn't just handle the middleware it also handles registering multiple MikroOrm to allow use of the MikroOrms token for DI, which is why I thought it would be better to rename it to multiple.
the user still needs to do the MikroOrmModule.forRoot calls explicitly, right? sounds like an implementation detail to me, from user point of view, it doesn't do anything else than the middleware i would say.
strictly speaking, the renaming of various symbols and types is a breaking change, i would rather keep the old naming everywhere
the user still needs to do the
MikroOrmModule.forRootcalls explicitly, right? sounds like an implementation detail to me, from user point of view, it doesn't do anything else than the middleware i would say.strictly speaking, the renaming of various symbols and types is a breaking change, i would rather keep the old naming everywhere
Yes the user would be required to MikroOrmModule.forRoot but that would always be the case for each database. But going along with the export changes to expose the MikroOrms (https://github.com/mikro-orm/nestjs/pull/88) and that this specific middleware is only used for multiple database it makes sense for the module to be named more scope to used for mutiple database. Also I thought there would might be a case where someone could want to use the MikroOrms token to get all databases but might not require the middleware (was planning to do after as it did make sense as part of this PR to disable the middleware) and use the DI scopes.
Yeah I understand adding the new MULTPLE_MIKRO_ORM_MODULE_OPTIONS symbol and type is a breaking change but I wanted those options to be closer to the module due to the renaming and it being scoped to multiple databases, but as this should be used internally I didn't think this would be an issue. Either way I am happy to continue using the old symbol and alias the old type for removal in the future.
@B4nan, I have added additional tests and a test to show the issue arising; I have also found that this issue also occurs with the original middleware registration within mikro-orm-core.module and I think the fix is to move the middleware registration up to mikro-orm.module but I think I would do this in another PR
looks like your approach is not working with nest 11
i will give it ago with nestjs 11
maybe the problem you were trying to solve is no longer valid with v11? that's the first thing I would try to verify
maybe the problem you were trying to solve is no longer valid with v11? that's the first thing I would try to verify
Yeah I notice some module changes and will go through it, if its fixed I will just leave the tests in to make sure its working to what I was expecting
maybe the problem you were trying to solve is no longer valid with v11? that's the first thing I would try to verify
Yeah I notice some module changes and will go through it, if its fixed I will just leave the tests in to make sure its working to what I was expecting
I've done some quick investigation on this, so with nestjs 11 they have changed the way how module distances are calculated with global modules and with the latest change it makes the global module middleware get executed last. I will raise an issue on the nestjs repo to see what they say.
maybe the problem you were trying to solve is no longer valid with v11? that's the first thing I would try to verify
The issue has been resolved https://github.com/nestjs/nest/releases/tag/v11.0.5.
With the fix Global Modules should execute first now and think we should keep the code fix so it works for nestjs 10.
@B4nan I have updated the peerDependency for nestjs 11 to go from ^11.0.5 so anyone installing shouldn't be using anything lower
why? because of something that was present for years now, in 10.x versions that are still supported? i would keep the peer deps loose, its up to the user to stay up to date. we can enforce this in v7 where we disallow older nest versions, but now it feels a bit weird, especially since it doesn't affect everyone (as mentioned above, the real-world example app still works just fine without any changes, even with lower versions).
do i get it right that things without your PR worked fine, and with the changes you did here it broke with v11 before 11.0.5?
do i get it right that things without your PR worked fine, and with the changes you did here it broke with v11 before 11.0.5?
My changes were mainly to resolve the issue in 10.x where the middleware registered in a global module it could be executed later and the test I made here we were able to catch an issue in nestjs 11 where it made the issue worse due to a bug where it made the global module middleware execute last hence why we should force to go from ^11.0.5 cause even without my fix it would cause the request context be executed last but happy to keep it loose at ^11.0.0 if you want it to be up to the users responsibility.
If we get this fix in then for nestjs 10.x we can remove this line https://github.com/mikro-orm/nestjs-realworld-example-app/blame/a61ea041db0d0cf24f32f49b262c5bfecd6a726d/src/app.module.ts#L32 in your example project
If we get this fix in then for nestjs 10.x we can remove this line mikro-orm/nestjs-realworld-example-app@a61ea04/src/app.module.ts#L32 (blame) in your example project
Oh, now that's interesting, didn't realize it's about this. Agreed that it makes sense to merge it in.
Let's keep it I guess, since the bump is not released out, I guess its fine to enforce first usable version.