-
-
Notifications
You must be signed in to change notification settings - Fork 153
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
chore(deps): upgrade @nestjs/axios to 3.0.1 #722
chore(deps): upgrade @nestjs/axios to 3.0.1 #722
Conversation
package.json
Outdated
"@nrwl/node": "12.10.1", | ||
"@nrwl/workspace": "12.10.1", | ||
"@nrwl/workspace": "17.0.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 wouldn't recommended updating nx directly in the package.json. Shouldn't we use nx migrate
(https://nx.dev/core-features/automate-updating-dependencies) as that usually does much more work than just updating 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.
I'm sorry, I should have read nx's document and shouldn't include nx update in this PR.
I only updated something related to the issue
8afad42
to
8d9cc25
Compare
package.json
Outdated
"@nuxtjs/opencollective": "0.3.2", | ||
"axios": "1.6.0", |
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 do you set a specific axios
version here? If you leave it out using projects can just set their own axios
version in future.
If it needed for testing it should just be added to devDependencies
like on @nestjs/axios
then the axios version would no "bleed" to child projects
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.
Testing can be done without it.
But if I remove axios, an warning occurs like below.
warning " > @nestjs/axios@3.0.1" has unmet peer dependency "axios@^1.3.1"
Is it okay to ignore this or should I add axios to devDependencies?
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.
as this is a library I would say this warning is okay as the consuming package should install its axios version
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 see, I removed axios
from dependencies
8d9cc25
to
ebf144e
Compare
Thanks for putting this together. Will it be released after this PR is merged? context GHSA-wf5p-g6vw-rhxx |
can you please take a look when you've time? thanks again for the PR. |
update @nestjs/axios and other packages with incorrect peer dependency
ebf144e
to
c4a74e9
Compare
@wing328 |
still some failed tests: https://github.com/OpenAPITools/openapi-generator-cli/actions/runs/7077821666/job/19262635518?pr=722 please take a look when you've time |
2b520c7
to
5627778
Compare
@nestjs/axios@3.0.1 needs to be installed with axios
5627778
to
bcb89b5
Compare
@wing328 |
Just reran the workflow. Let's see how that goes |
unfortunately the tests still failed:
|
0327d79
to
6863baa
Compare
@wing328 Sorry for taking a while to fix ci failure. |
@wing328
|
done but the tests still failed with the same error message |
e435eb0
to
83b72f9
Compare
@wing328 |
done but got the same error |
I don't know a lot about NX, but this seems like a bug in NX related to the implicit dependency functionality.
Maybe upgrading NX first would help? 🤷 Or is there a way to specify an explicit dependency with NX? |
@simonhammes thanks for the suggestion @axtx4869 can you please also update NX as part of this PR? (I saw the comment from @wvanderdeijl before) |
Any updates on this issue? I see that NX (17.2.x) now has axios@1.6.5 as a dependency which fixes the vulnerability there. |
@elmarbeckmann do you mind filing a PR on top of this change with the NX upgraded to newer version? |
here you go, made a new PR. #730 |
closed via #730 |
update @nestjs/axios and other packages with incorrect peer dependency
fix #719