-
Notifications
You must be signed in to change notification settings - Fork 943
Update grpc-js to 1.3 #4871
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
Update grpc-js to 1.3 #4871
Conversation
|
Binary Size ReportAffected SDKs
Test Logs
|
Size Analysis Report |
Please add a changeset |
@Feiyang1 The size increase seems to be from the inclusion of AppCheck. Is that a good assumption? |
packages/firestore/package.json
Outdated
@@ -71,7 +71,7 @@ | |||
"@firebase/logger": "0.2.6", | |||
"@firebase/util": "1.0.0", | |||
"@firebase/webchannel-wrapper": "0.4.1", | |||
"@grpc/grpc-js": "^1.0.0", | |||
"@grpc/grpc-js": "1.3.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.
Do you want to use ^
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.
Makes sense. I didn't realize we only use fixed versions for our own dependencies.
I think so. the base is a commit before appcheck was merged into master. The diff should be much smaller if you rebase the PR. |
Hm, I am not able to run the yarn update right now ("error An unexpected error occurred: "expected workspace package to exist for "inquirer"""). I will ping you once I updated the yarn.lock |
88ab41b
to
bc3814b
Compare
@Feiyang1 CI finally passed. I had to update some random types after removing yarn.lock |
@@ -27,7 +27,7 @@ | |||
"rollup-plugin-typescript2": "0.29.0", | |||
"@rollup/plugin-virtual": "2.0.3", | |||
"webpack": "4.44.2", | |||
"@types/webpack": "4.41.25", | |||
"@types/webpack": "5.28.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.
Is it required to pass CI? This is a wrong change because we are not using webpack@5. I will be okay with it if it's required to pass CI for some unknown reason. I don't want us to waste time on it and we should be updating to webpack@5
soon.
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 am not sure if 5.x is required, but the version that was installed without the yarn.lock file caused compilation errors. I can try an older 4.x version, but we should be safe since the code itself did not change.
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.
The problem is here (from the original yarn.lock):
"@types/webpack@4.41.25":
version "4.41.25"
resolved "https://registry.npmjs.org/@types/webpack/-/webpack-4.41.25.tgz#4d3b5aecc4e44117b376280fbfd2dc36697968c4"
integrity sha512-cr6kZ+4m9lp86ytQc1jPOJXgINQyz3kLLunZ57jznW+WIAL0JqZbGubQk4GlD42MuQL5JGOABrxdpqqWeovlVQ==
dependencies:
"@types/anymatch" "*"
"@types/node" "*"
"@types/tapable" "*"
"@types/uglify-js" "*"
"@types/webpack-sources" "*"
source-map "^0.6.0"
Webpack 4.x does not compile against tapable 2.x. The old version was 1.06 (before removing the lock file).
These are some of the errors:
../../node_modules/@types/webpack/index.d.ts:1106:32 - error TS2314: Generic type 'HookMap<H>' requires 1 type argument(s).
1106 callAnyMember: HookMap;
~~~~~~~
Updates minimum GRPC version to 1.3.
Fixes #4851