lehh / nestjs-soap

Nestjs module wrapper for soap npm package
MIT License
21 stars 15 forks source link

chore: add compatibility with nestjs 9 #24

Closed ammarnajjar closed 2 years ago

lehh commented 2 years ago

Hi @ammarnajjar, @martinepic and @marceloformentao-hotmart

Sorry for the very late reply on this. I tried to run it with a nestjs 6.10 application and it has some type errors related to the DynamicModule when importing the module. Would any of you be kind enough to test it and see if this is happening in your environment as well? I need to confirm this so I will have to drop the v6.10 compatibility on the next version if this doesn't have a fix.

ammarnajjar commented 2 years ago

Hi, Unfortunately I don't have a working environment with nestjs version ^6.10.0 to test against it.

I tried to make a minimum nestjs 6 project here: https://github.com/ammarnajjar/nest6-project but I don't see any errors, so maybe I am missing sth?

If you please @lehh have a look, and suggest the change to trigger the errors you mentioned if possible.

Thanks

ammarnajjar commented 2 years ago

A question though: 😄

@lehh , does the nestjs 6 compatibility issue block this PR?

marceloformentao-hotmart commented 2 years ago

I think so @ammarnajjar, if it's not compatible with v6 it should bump major version to avoid breaking changes for ppl using this with nest v6.

lehh commented 2 years ago

Hi, Unfortunately I don't have a working environment with nestjs version ^6.10.0 to test against it.

I tried to make a minimum nestjs 6 project here: https://github.com/ammarnajjar/nest6-project but I don't see any errors, so maybe I am missing sth?

If you please @lehh have a look, and suggest the change to trigger the errors you mentioned if possible.

Thanks

Hi @ammarnajjar,

Thanks for creating a repository for this!

Yeah, you will need to reference the nestjs-soap from your PR repo. Example:

"dependencies": {
    "@nestjs/common": "6.10.0",
    "@nestjs/core": "6.10.0",
    "@nestjs/platform-express": "6.10.0",
    "nestjs-soap": "../nestjs-soap",
    "reflect-metadata": "^0.1.13",
    "rimraf": "^3.0.2",
    "rxjs": "^6.6.0"
  },

Then you will start to get an error with the DynamicModule typing in the app.module.ts imports.

I'm trying to find some fix for it. In the worst case, I will need to do what @marceloformentao-hotmart said. I'm trying to avoid this because I'm not going to have time to maintain two different versions.

lehh commented 2 years ago

It seems there was a type update on nestjs v8.4.0. Since nestjs-soap was using nestjs v8.1, it's working fine in the current version.

So, I'm going to fix nestjs version on 8.3.1 (#26) for the v2 package and I will release a v3 that will only be compatible with nestjs ^8.4.0 and ^9.0.0. I believe that will be the solution for this 🤷🏻 .

lehh commented 2 years ago

Done guys. Thanks for the help and contribution!