-
Notifications
You must be signed in to change notification settings - Fork 13
Update dependencies snyk reports as vulnerable #13
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
base: master
Are you sure you want to change the base?
Conversation
98059d2 to
b5b6eae
Compare
client.js
Outdated
| }, circuitBreakerParams); | ||
| this.disyuntor = new Disyuntor(circuitBreakerParams); | ||
|
|
||
| this._protectedRequest = (...args) => { |
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.
Why to use this format instead of Disyuntor.wrapCallbackApi which works pretty similar as before?
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.
Because we use the old disyuntor.reset() which is now available only if we use the class Disyuntor.
https://github.com/limitd/node-client/pull/13/files#diff-215b25d5bf5c0b4623ad1a2b62456f60L127
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.
If you check the Note here: https://github.com/auth0/disyuntor#complex-scenarios this api supports only promise-returning functions and I had to write a similar wrapCallbackApi
by myself here.
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 is a pity the code is really complex for such a simple use case. Maybe we should add the feature to disyuntor before making the change.
package.json
Outdated
| "dependencies": { | ||
| "async": "^2.3.0", | ||
| "disyuntor": "^2.0.0", | ||
| "disyuntor": "^3.4.2", |
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.
We found several critical issues in this version of disyuntor, please update to 3.4.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.
Is there any vulnerable code in the version 2 of disyuntor? I'd advice not to update otherwise, 2.0.0 is really stable whereas 3 is not so battle tested and changes a lot + limitd is a service where each deployment involves downtime
client.js
Outdated
|
|
||
| this._protectedRequest = (...args) => { | ||
| const callback = args[args.length - 1]; | ||
| this.disyuntor.protect((resolve, reject) => { |
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.
why resolve, reject here?
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.
it is overridden bellow
client.js
Outdated
| const newArgs = args.slice(0, -1) | ||
| this._directRequest(...newArgs, (err, ...args) => { | ||
| if (err) {return reject(err); } | ||
| resolve(...args); |
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.
Resolve takes a single value, if you need to support more than one then you need to resolve with an array and the in the "then" you can unwrap the array.
new Promise((resolve) => { resolve('a', 'b'); })
Promise {<resolved>: "a"}__proto__: Promise[[PromiseStatus]]: "resolved"[[PromiseValue]]: "a"
3cfab3d to
07e38e7
Compare
vulnerability reports: