feat: add support for nestjs@8.0.0 #29
Conversation
Started looking into the code before I read the OP :] Ok understood, thanks for the insights. If we can make this work without BCs, then it would be great even if it would be quirky solution. We can have something cleaner in v5. |
StrikeForceZero
force-pushed
the
nestjs8
branch
2 times, most recently
from
169418b
to
050e14f
Compare
| TReturnValue, | ||
| >(moduleName: TModuleName, returnFn: (module: EntityManagerExportObj<TModuleName>) => AsyncOrSync<TReturnValue>): Promise<[Awaited<TReturnValue>] | []> { | ||
| try { | ||
| const module = await import(moduleName); |
There was a problem hiding this comment.
do i read it correctly that once we move to native ESM, we cant use forRoot and it will have to be forRootAsync? is there any real difference apart from the name? i mean it feels like just our own logic that offers two ways, but for nest it is the same, right? so once that happens, we basically drop forRoot and rename forRootAsync to just forRoot
There was a problem hiding this comment.
I believe that is correct.
In fact, forRootAsync wasn't marked as async prior to this PR, so the signature was the same by not returning a promise.
There was a problem hiding this comment.
Strange. Testing forRootAsync might have uncovered some code paths that do not have tests on them and are causing errors in my project. I'm going to do some investigation and report back.
There was a problem hiding this comment.
oh maybe I'm being naive, forRootAsync doesn't have to be async; it can return arrays of Promises for exports and providers that resolve as DynamicModules and Providers respectively.
Maybe its worth taking the time to not await all those promises so they are being loaded concurrently instead of sequentially.
There was a problem hiding this comment.
I was wrong, providers do not appear to support Promises, only exports.
So what I have is probably the best I can come up with for now.
Edit: and any performance impact from a quick inspection appears to be negligible if any awaiting sequentially. Not to mention that users probably wont have both modules available.
There was a problem hiding this comment.
forRootAsync usually implies is that it will be able to consume async factory providers that provide config for the module, not that the method will be async.
If you want the dynamic module to be async itself, you should be able to mark both forRoot and forRootAsync as async methods and then await the whenModuleAvailableAsync method. This would also make the whenModuleAvailable method useless.
There was a problem hiding this comment.
@B4nan was there anything actionable here for me prior to getting this accepted? or did we want to go a different route?
There was a problem hiding this comment.
I was hoping to get rid of the require statement, not having two ways for the same thing. as @jtsshieh noted, it should be possible to just define the method as async and simplify the code.
There was a problem hiding this comment.
ah ok, thanks for clearing that up for me. 53c0a4d should reflect that now
|
Hey all, I'd love to help move this effort along in any way I can. I have a project that I want to use NestJS and Kafka (requires NestJS@8) with MikroORM (requires NestJS@7 currently). Can I help test or advise at all? |
|
@ByronDDelpinal it would help a lot if you could verify that the changes done in the PR are backwards compatible, so we can call this semver-minor and not a BC. |
StrikeForceZero
force-pushed
the
nestjs8
branch
from
b7e24de
to
53c0a4d
Compare
|
Hey, I too am waiting on NestJS v8 support for a project of mine. Is there anything I could do to assist? |
|
Is there any way I could provide help here as well? |
|
@B4nan Hey, any updates on it? Let us know if any help needed |
|
@Injectable()
export class EntityManagerService implements OnModuleInit {
constructor(private moduleRef: ModuleRef) {}
async onModuleInit() {
// const type = await figureOutWhichEntityManagerTypeToUse()
// if sql entity manager is available, you register it into the module as:
await this.moduleRef.create<SqlEntityManager>(sqlEntityManager)
// or if mongo is available
await this.moduleRef.create<MongoEntityManager>(mongoEntityManager)
}
}
EDIT: I've tried this, and wasn't able to get it to work. |
|
I just shipped the contents of this PR to the edit: Looks like this it not working for custom repositories without edit2: found a hack to get around it by checking the global metadata storage, so will work only for decorators, still better than nothing |
Fixes usage of custom repositories without `@InjectRepository` decorator.
B4nan
changed the title
|
v4.3 stable is out |
|
I am receiving this error after upgrading to v4.3 stable + Nest 8 and not exactly sure what to do to resolve it. v4.3 + Nest 7 still seems to work fine though:
import { Module } from "@nestjs/common"
import { MikroOrmModule } from "@mikro-orm/nestjs"
@Module({
imports: [MikroOrmModule.forRoot()],
})
export class AppModule {} |
|
This is exactly how the realworld example does it too, and it works just fine in there. My guess is you have some nest packages still on v7, all of them need to be upgraded. If you want more help, prepare full reproduction. Or check out the realworld example and try to compare what you are doing differently. https://github.com/mikro-orm/nestjs-realworld-example-app |
|
@B4nan it seems to be a yarn workspaces issue with hoisting. I made the following change to package.json and it seems to be working now. It might be a good note to mention in the README because it is not immediately obvious from Nest's error output: Add this to "private": true,
"workspaces": {
"nohoist": [
"@mikro-orm/nestjs"
]
}, |
|
That sounds more like a workaround for a misconfigured monorepo (e.g. some undeclared peer deps), could you provide a repro for that so I can review it first? I don't want to add hints to the docs without proper understanding of the problem. Ideally we should have an example app for monorepos too. |
|
app.module.ts: Output when i run 'npm start:dev': Package versions: I'm missing something? |
I'm also using monorepo and for the strangest reason, this worked. @msheakoski do you have any idea why? Maybe it has to do with the location of the |
resolves: #27 (maybe)
As mentioned in the issue nestjs@^8.0.0 uses class instances instead of strings.
Took the suggestion to remove the driver specific imports in favor of the one from
knexTested minimally but might be all that's needed?
The 2 occurrences I identified as problematic are highlighted in this MR.I actually don't really intend on this being pulled in. I think passing in the respectiveEntityManagersub class toforRootwould be more appropriate and a lot cleaner, but this might help give a starting point.