-
Notifications
You must be signed in to change notification settings - Fork 335
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
feat: Update protobufjs (and peer dependencies) to ^7 #874
Conversation
There is one failing test where I'd appreciate getting some help for because I'm unfamiliar with grpc and the intended behaviour. For what it's worth, downgrading |
This is great @threema-lenny ! This failing test:
Is probably due to our So far we've been able to intercept
And So, worst-case scenario is that they removed this wrappers API entirely in protobufjs 7, or best-case scenario is that the API and/or maybe even just the import/export changed (maybe due to a bundling/packaging change from 6 to 7?), but really not sure... Maybe that's enough guidance for you to run with it from there? Also fwiw/fyi that @boukeversteegh is our resident Struct/well-known-types effort, just as an FYI to him that our Thanks! |
I think I'll need to pass on that task because I simply don't know anything about gRPC, NestJS and the |
@threema-lenny Np! I poked around a little bit, thinking that maybe protobufjs broke the I pushed up the |
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.
Awesome! Thanks @threema-lenny !
# [1.153.0](v1.152.1...v1.153.0) (2023-07-12) ### Features * Update protobufjs (and peer dependencies) to ^7 ([#874](#874)) ([7f979a7](7f979a7))
🎉 This PR is included in version 1.153.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Oh dear, yeah, those side effects are evil. Thanks a lot, @stephenh! |
Thanks @stephenh and @threema-lenny! |
Hello, thank you for the work! After installing Lines 2 to 3 in ce064d0
Line 15 in ce064d0
Script to reproduce: % rm -rf ts-proto-test \
&& mkdir ts-proto-test \
&& cd ts-proto-test \
&& npm init -y \
&& npm i -S ts-proto@1.153.0 \
&& cat ./node_modules/ts-proto-descriptors/package.json | jq .dependencies
Wrote to /Users/akos.kitta/Desktop/dev/__trash/ts-proto-test/package.json:
{
"name": "ts-proto-test",
"version": "1.0.0",
"description": "",
"main": "index.js",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"
},
"keywords": [],
"author": "",
"license": "ISC"
}
added 25 packages, and audited 26 packages in 2s
1 package is looking for funding
run `npm fund` for details
1 high severity vulnerability
To address all issues, run:
npm audit fix
Run `npm audit` for details.
{
"long": "^4.0.0",
"protobufjs": "^6.8.8"
}
% npm audit
# npm audit report
protobufjs 6.10.0 - 7.2.3
Severity: high
protobufjs Prototype Pollution vulnerability - https://github.com/advisories/GHSA-h755-8qp9-cq85
fix available via `npm audit fix`
node_modules/ts-proto-descriptors/node_modules/protobufjs
1 high severity vulnerability
To address all issues, run:
npm audit fix What am I doing wrong? Thank you! |
You're probably doing nothing wrong, I suspect it's the cyclic dependency I hinted at. I'm assuming we need to bump |
Yep! I forgot about that; I think it should be fixed with this PR that I just merged: |
After installing However, one of my tests started to fail with Click to see error
|
Thanks for the bug report @dankeboy36 ; I just pushed out a fix, can you see if that works? Thanks! |
It's working great now with |
I scanned the dependencies we have to protobufjs files (mostly reader/writer) and there seems to have been no changes requiring any further action.
I've also scanned for peer dependencies to protobufjs 6 and updated those accordingly (best effort).
Note: We have a cyclic dependency between
ts-proto
andts-proto-descriptors
whose protobufjs dependencies affect one another, so now we have two different protobufjs versions in thenode_modules
folder. Not sure how you want to handle this, @stephenh.Resolves #834