feat!: Replace protobuf.js #1058
Conversation
Replaces Reader from protobuf.js with BinaryReader from @bufbuild/protobuf/wire. Next up: Replace Writer with BinaryWriter, and sort out Long.js Note: This feature should be behind a plugin option, but it isn't yet.
protobuf.js' Reader uses Node.js' Buffer when running on Node.js. This means that tests were expecting a Buffer, even when they do not set env=node. BinaryReader from @bufbuild/protobuf/wire always uses Uint8Array, causing the expectations to fail. To solve the problem, we update the tests to expect Uint8Array. There is one odd case in integration/unknown-fields/unknown-fields-test.ts that required a workaround (see comments). The workaround can be removed after migrating Reader as well. With this change, all tests pass, except simple-esmodule-interop.
stephenh@c307550 changed generated code. Merging it in reverted our changes.
The PR added new tests, and we need to re-generate the proto files.
BinaryReader already returns a BigInt for 64-bit integers. We don't need to convert it. If BigInt is not supported in the current runtime (Safari was the last to catch up in 2020), it returns a string. In that case, it isn't possible to convert to a BigInt anyway. This should be a regression, because the old code relied on BigInt as well.
protobuf.js automagically loads Long.js from node_modules, and uses it in Reader. If the magic fails (easily happens with bundlers), it falls back to an alternative and returns Number instead of Long. Since the generated code no longer uses Reader, it is no longer necessary to explicitly configure protobuf.js to use Long.js
The conversion function just calls toString() on the value. We can do that without a conversion function.
| function longToNumber(int64: { toString(): string }): number { | ||
| const num = ${bytes.globalThis}.Number(int64.toString()); |
There was a problem hiding this comment.
I've verified that this check is reliable in #1057 (comment).
Since the package is not published yet, use the tsconfig option "path" to redirect imports from the npm package to the local sources.
And in ts-proto-descriptors, replace long and protobufjs with @bufbuild/protobuf
timostamm
force-pushed
the
replace-protobuf.js
branch
from
d603481 to
7ef0c73
Compare
|
@stephenh, I took care of the follow-ups. This is using an alpha of @bufbuild/protobuf v2. I promise we won't break it if you want to publish as is. A stable v2 is about a month out in case you prefer to wait.
|
|
Amazing, thank you @timostamm ! |
|
A beta of |
|
Awesome! Thanks for the update @timostamm ! |
|
@timostamm @stephenh It looks like |
|
This is amazing, as usual, @timostamm , thank you for the bump to the latest! I merged this to |
|
Just wanted to say thanks again @timostamm ! Really appreciate the help on this PR, and looking forward to @bufbuild/protobuf being a much better runtime for us! 🙏 |
|
Sorry, I was out for a couple of days. This was fun 😃 Congratulations on the v2 release! |
This PR replaces protobuf.js with @bufbuild/protobuf in generated code. The libraries are used for low-level wire encoding. Since they have a similar API, the changes to main.ts are relatively simple.
The main advantage is that Long.js is no longer a dependency, unless you explicitly want to use Long.js (plugin option
forceLong=long).The signatures of
encodeanddecodechange - they no longer useReaderandWriterfrom protobuf.js, but the counterparts from@bufbuild/protobuf. This will be a breaking change for users that manually pass in readers, or rely on the returned writer to be a specific type.Follow-ups:
Replace0d763e9Readerin src/schema.tsRe-generate code for3ae8889ts-proto-descriptorsMove7ef0c73protobufjsfrom dependencies to devDependencies