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

Support for Nestjs v11 #197

Closed
snigdha920 opened this issue Jan 20, 2025 · 8 comments · Fixed by #198
Closed

Support for Nestjs v11 #197

snigdha920 opened this issue Jan 20, 2025 · 8 comments · Fixed by #198

Comments

@snigdha920
Copy link
Contributor

snigdha920 commented Jan 20, 2025

NestJS released a new major version today which breaks the forFeature type, I cannot do MikroOrmModule.forFeature([MyEntity]) anymore:

@Module({
  imports: [MikroOrmModule.forFeature([MyEntity])],
  providers: [MyEntityService],
  exports: [MyEntityService],
})
export class MyEntityModule {}

Error:

Type 'DynamicModule' is not assignable to type 'Type<any> | ForwardReference<any> | DynamicModule | Promise<DynamicModule>'.
  Type 'import("/Users/snigdhasingh/Development/btp/node_modules/@mikro-orm/nestjs/node_modules/@nestjs/common/interfaces/modules/dynamic-module.interface").DynamicModule' is not assignable to type 'import("/Users/snigdhasingh/Development/btp/node_modules/@nestjs/common/interfaces/modules/dynamic-module.interface").DynamicModule'.
    Types of property 'imports' are incompatible.
      Type '(DynamicModule | Type<any> | Promise<DynamicModule> | ForwardReference<any>)[] | undefined' is not assignable to type '(Type<any> | ForwardReference<any> | DynamicModule | Promise<DynamicModule>)[] | undefined'.
        Type '(DynamicModule | Type<any> | Promise<DynamicModule> | ForwardReference<any>)[]' is not assignable to type '(Type<any> | ForwardReference<any> | DynamicModule | Promise<DynamicModule>)[]'.
          Type 'DynamicModule | Type<any> | Promise<DynamicModule> | ForwardReference<any>' is not assignable to type 'Type<any> | ForwardReference<any> | DynamicModule | Promise<DynamicModule>'.
            Type 'Promise<DynamicModule>' is not assignable to type 'Type<any> | ForwardReference<any> | DynamicModule | Promise<DynamicModule>'

Probably more things could be broken, opening this issue to track when mikro-orm/nestjs will support v11, so I can upgrade my project to v11 as well

@B4nan
Copy link
Member

B4nan commented Jan 20, 2025

Try using skipLibCheck, the update is actually a few days old already, and I already merged it in https://github.com/mikro-orm/nestjs-realworld-example-app where it came via renovate bot, the tests are still passing just fine, so I can imagine it's just a type issue.

Surely something to fix, so thanks for reporting. Looking at the error, it feels like you ended up with two copies of @nestjs/common, possibly because of our peer dependency constraints not allowing nest 11 (which might be why the tests still work in the real-world example).

@snigdha920
Copy link
Contributor Author

snigdha920 commented Jan 20, 2025

I opened a PR here to update the version: #198 and the tests worked locally after the update so there's no other changes required to use v11. Think we can run the CI on the PR, and then merge it?

For context, nestjs-prometheus had the exact same type issue, fixed by adding v11 as a peer dependency: willsoto/nestjs-prometheus#2392

@tsangste
Copy link
Contributor

@B4nan as a note before releasing nestjs 11 support, a change needs to be made to

(isNestMiddleware(consumer) && usingFastify(consumer) ? '(.*)' : '*');
as express 5 changes the way it handles wildcards as noted in the nestjs 11 migration guide https://docs.nestjs.com/migration-guide#express-v5

@B4nan
Copy link
Member

B4nan commented Jan 22, 2025

hmm so we'll need to find a way to make this conditional, we cant just break it for nest v10 outside of major bump (and i am not willing to ship v7 just for this)

@B4nan B4nan reopened this Jan 22, 2025
@tsangste
Copy link
Contributor

hmm so we'll need to find a way to make this conditional, we cant just break it for nest v10 outside of major bump (and i am not willing to ship v7 just for this)

Yeah I was thinking of switch of some sort so it works for both 10 and 11. I am trying to find out whether there is a way we can check the express version but think if all comes to it we might need a enum or flag to opt in?

@B4nan
Copy link
Member

B4nan commented Jan 22, 2025

it would be enough to detect nest version i guess. we could do that by checking for some method or class or symbol that wasn't there before, or similar things. having flags would mean we need to make this opt in, people would most likely not find out unless they fall into some issues.

reading versions from the projects package.json is another way out. better than having a flag and delegating this to users (but we can still have a flag to override the detection)

@tsangste
Copy link
Contributor

it would be enough to detect nest version i guess. we could do that by checking for some method or class or symbol that wasn't there before, or similar things. having flags would mean we need to make this opt in, people would most likely not find out unless they fall into some issues.

reading versions from the projects package.json is another way out. better than having a flag and delegating this to users (but we can still have a flag to override the detection)

I noticed in the migration document the following:

// In NestJS 11, this will be automatically converted to a valid Express v5 route.
// While it may still work, it's no longer advisable to use this wildcard syntax in Express v5.
forRoutes('*'); // <-- This should not work in Express v5

so we could potentially leave it until v7?

@B4nan
Copy link
Member

B4nan commented Jan 22, 2025

yes, i thought you actually saw a problem somewhere. the real-world example app works fine with v11 without any changes, so i guess we can close this one again

@B4nan B4nan closed this as completed Jan 22, 2025
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 a pull request may close this issue.

3 participants