-
Notifications
You must be signed in to change notification settings - Fork 33
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
fix reflector issue in nestjs v8 #13
Conversation
@tukusejssirs PTAL. |
"reflect-metadata": "^0.1.12", | ||
"rimraf": "^2.6.2", | ||
"rxjs": "^6.3.3" | ||
"mqtt": "4.3.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, using 4.3.1
didn’t solve the issue. I still get Cannot set property length of #<Map> which has only a getter
. Latest commit that does not contain collections
dependency is 8aa2f8d8 (collections
were added in c92b877).
I understand this is not an issue of nest-mqtt
, but of mqtt
, but nevertheless it causes issues with other packages utilising nest-mqtt
/mqtt
and using the standard Node methods modified by collections
package.
Note that when nest-mqtt
is used alone, it works as expected, but when used with some other packages (say MikroORM), collections
package causes great troubles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@microud, actually, last version without collections
dependency is v4.2.8
and last commit without it is 8aa2f8d, therefore even 4.3.1
does not solve the issue (at least not for me). I wish they would remove collections
from mqtt
dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tukusejssirs I notice mqtt.js
has released version v4.3.3
which removed collections.js dependency. I will upgrade dependence.
I could remove mqtt dependence and let user decide which version should be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tukusejssirs I notice
mqtt.js
has released versionv4.3.3
which removed collections.js dependency. I will upgrade dependence.
No, it wasn’t removed yet, just its use diminished. I already tested it, but still cannot use it.
I could remove mqtt dependence and let user decide which version should be used.
That could be a way to deal it. Even @nestjs/microservices
has set mqtt
version to *
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There’s a new PR which removed the collections
dependency: mqttjs/MQTT.js#1396. It should fix my issues.
However, using *
would be better IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tukusejssirs Yes, remove lib dependence is the better way for nest wrapped module. It could break the binding relationship between nest module and base lib.
"@types/jest": "24.0.11", | ||
"@types/mqtt": "^2.5.0", | ||
"@types/node": "11.13.4", | ||
"@types/supertest": "2.0.7", | ||
"jest": "24.7.1", | ||
"prettier": "1.17.0", | ||
"reflect-metadata": "^0.1.12", | ||
"rimraf": "^2.6.2", | ||
"rxjs": "^6.3.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not install the deps (npm i
) unless updating rxjs
to the latest version (^7.5.1
). Otherwise (without any deps modification) the following error is thrown (node v17.0.1
, npm 8.1.1
, nest 8.1.6
):
npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR!
npm ERR! While resolving: [email protected]
npm ERR! Found: [email protected]
npm ERR! node_modules/rxjs
npm ERR! dev rxjs@"^6.3.3" from the root project
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peer rxjs@"^7.1.0" from @nestjs/[email protected]
npm ERR! node_modules/@nestjs/common
npm ERR! dev @nestjs/common@"^8.0.0" from the root project
npm ERR!
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
npm ERR!
npm ERR! See /home/ts/.npm/eresolve-report.txt for a full report.
npm ERR! A complete log of this run can be found in:
npm ERR! /home/ts/.npm/_logs/2022-01-05T16_27_20_121Z-debug.log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be because I already use a newer major version in my app. And I also noticed that many of the other deps are not up to date.
fix injection of
Reflector
not working on NestJS v8 anymore and update dependences to peer dependence.