Open AndKiel opened 1 year ago
Can you please provide a complete repro as a GH repository?
Have you tried it without ncc
? Could be some issue in there rather than here, since things seem to be working fine with regular tsc
build on my end.
constructor(entityManager: EntityManager) { this.entityManager = entityManager.fork(); }
This is a pretty bad idea, you are just getting around the validation, but still using a single global EM fork. This will almost certainly have unwanted side effects.
The reproduction above is complete (package.json
, tsconfig.json
, reproduction.ts
). You'd find nothing more in a repository. The only thing that can be added is any MongoDB v3 instance which can be easily created via Docker:
docker-compose.yml
version: "3.7"
services:
db-mongo:
image: "public.ecr.aws/docker/library/mongo:3"
healthcheck:
test: ["CMD-SHELL", 'mongo --eval ''db.runCommand("ping").ok'' localhost:27017/test --quiet']
interval: 10s
timeout: 2s
retries: 10
ports:
- "27017:27017"
If the version 5.1.2 works fine with ncc
and any version above does not, then I'm inclined to say some change here may be at fault.
This reproduction merely shows the error and how to cause it. It does not show the actual business use case which is AWS EventBridgeEvent lambda that gets/resolves various services using cached and initialized application context assigned to let
.
The reproduction above is complete (package.json, tsconfig.json, reproduction.ts). You'd find nothing more in a repository. The only thing that can be added is any MongoDB v3 instance which can be easily created via Docker:
I am asking because I want to spare my time, as I have a huge amount of other issues I need to review these days. I don't want to copy past things and create files.
Sounds like you value your time more than mine. That's fine, but don't expect me to also put more value on yours...
Thanks!
Maybe this one is related? https://github.com/vercel/ncc/issues/776
Doesn't seem related. I have no issues with decorators in different projects using exactly the same ncc version and other dependencies but with sequelize instead mikro-orm. And @mikro-orm/nestjs
v5.1.2 worked fine before.
You keep talking about @mikro-orm/nestjs
v5.1.2, but this package hasn't been released for quite some time, so I guess you mean the ORM packages, not the nest adapter?
No, I mean exactly what I said above and I wrote the same in the issue description. @mikro-orm/nestjs
v5.1.2 works. Upgrading to a higher version breaks built code execution.
Oh ok, so the problem is in this package. I was suspecting some changes I did in v5.7.13 released a few days ago. Will try to look into this later this week.
It appears that the issue is that it is calling the useFactory method without actually injecting any of the injected values. I ran into this with the config service not being injected after updating. I am not sure why it broke only on 5.7.13 (for me).
I've added an alternative reproduction to the reproduction repository - one using InjectRepository
instead of EntityManager
. This variant throws the error even when using ts-node to execute the file.
constructor(entityManager: EntityManager) { this.entityManager = entityManager.fork(); }
This is a pretty bad idea, you are just getting around the validation, but still using a single global EM fork. This will almost certainly have unwanted side effects.
Regarding this, what would be the proper way of doing things for applications using NestFactory.createApplicationContext
? What I mean is for example EventBridgeEvent lambdas where there are no HTTP requests so there is also no request context. This manual forking was my "workaround" because with InjectRepository
it threw ValidationError
. Is passing the scope
property to the forRootAsync
the proper way?
Is passing the scope property to the forRootAsync the proper way?
I am not sure if that would help, it controls the nestjs DI scopes, and since there are no requests, I'd expect there won't be any request context in the DI either, but maybe I am wrong, haven't used nest in a long time.
You could create the context explicitly via RequestContext.createAsync()
or use the @UseRequestContext
decorator.
https://mikro-orm.io/docs/usage-with-nestjs#request-scoped-handlers-in-queues
The second test is missing forFeature
call, which registers the repositories to the DI. If you import that in the MongoModule
where you are using it, then it starts to work (including the built version).
@Module({
providers: [MongoService],
imports: [
MikroOrmModule.forFeature([MongoEntity]),
],
})
export class MongoModule {}
Ah, you're right, silly mistake.
I fiddled with scope
a bit and found out that:
scope
is not passed, using the repository
throws ValidationError
. More or less expected because there is no request scope as mentioned in previous comments.scope
is passed:
applicationContext.get
works but repository
becomes undefined (it's not injected) and .resolve
must be used instead. This seems like a bug.MongoService
instance gets EntityManager
with _id
incremented by 1. This seems correct and wanted behaviour so that application behaves as if there was a request context even if there isn't one.It seems to be working fine both with ts-node and with code built via ncc. Maybe usage with NestFactory.createApplicationContext
should be documented somehow if the above is how it's supposed to work.
Now, should the initial reproduction be considered an actual bug or an incorrect usage due to the above? If it's incorrect usage, then .get
behaviour may need investigation as to why repository is not being injected at all when scope is present.
The thing is I am not completely sure how it is supposed to be working in nest di, it's been years since I used it in an actual project (same for any web app, I no longer develop those). I checked how the official nest/typeorm adapter registers things, and it feels pretty much the same as we do here, especially when it comes to exports
field.
It seems to be working fine both with ts-node and with code built via ncc. Maybe usage with NestFactory.createApplicationContext should be documented somehow if the above is how it's supposed to work.
That's good news, could you PR that somewhere? e.g. the readme here, I can propagate it to mikro-orm.io myself.
There are some edge cases around this. For example, if I make the whole MongoService in my reproduction have a scope but skip it in MikroOrmModule.forRootAsync, then I still get ValidationError when using the repository. As if injection ignored the fact that the whole service is scoped. No idea if it should somehow propagate without explicit MikroOrmModule scope being defined or not. At least I found a way to make it work with InjectRepository and solve my use-case. And it doesn't require manual forking as a workaround.
For example, if I make the whole MongoService in my reproduction have a scope but skip it in MikroOrmModule.forRootAsync, then I still get ValidationError when using the repository. As if injection ignored the fact that the whole service is scoped.
I am more than sure you need to use the scope
option of the forRoot
call, otherwise the services are registered as singletons, its not relevant that your MongoService
(which is a leaf, not a dependency of those) is request scoped - you don't import that into the ORM services, so they don't have to he request scoped at all.
Describe the bug
EntityManager
(MongoEntityManager
) dependency injection cannot be resolved when using@mikro-orm/nestjs
>5.1.2 with.forRootAsync
andPinoLogger
. It works fine without injecting the logger into customuseFactory
. Happens only when executing built code.Stack trace
To Reproduce
package.json
tsconfig.json
reproduction.ts
Expected behavior I expect the
EntityManager
to get injected correctly and work as it was working before in version 5.1.2.Additional context The error happens only when running a built .js file. It does not occur when running .ts file via ts-node. The
mongodb
dependency is locked to version 4.1.4 because of an old MongoDB v3 instance that needs to be connected to.Versions