search

bufbuild / protobuf-es

Protocol Buffers for ECMAScript. The only JavaScript Protobuf library that is fully-compliant with Protobuf conformance tests.
Apache License 2.0
1.09k stars 61 forks source link

Tree-Shaking for `fields` #769

Closed liril-net closed 4 months ago

liril-net commented 5 months ago

Hi there, i am working with this fantastic package in my project, but i found it not works with tree-shaking and as project grows, it becomes so large for the output files.

I find that maybe the static readonly fields: FieldList make the rollup tree-shaking not works as it initialized.

Can we make it as a function such as getFields() to make it lazy init to let the tree-shaking works?

timostamm commented 5 months ago

Hey Cyril, I think you're right that the call to newFieldList in generated TS can impact tree-shaking:

https://github.com/bufbuild/protobuf-es/blob/2b2861b9e395c96d1450ed569b1cd28756fe04ca/packages/protobuf-test/src/gen/ts/extra/example_pb.ts#L63

I believe that adding a /*@__PURE__*/ annotation right before the call expression should help (see https://github.com/bufbuild/protobuf-es/pull/470 for reference).

But before we make this change, we need to verify that it actually works as expected (previous changes were tested with https://github.com/connectrpc/examples-es/tree/main/bundle-size). This will take us a bit to look into, but it looks worthwhile to me.

Thanks for filing the issue!

smaye81 commented 5 months ago

Hi @liril-net. We tested adding this annotation to the newFieldList call, but we aren't seeing any effect with Rollup (or most other bundlers). The only bundler that sees an improvement is esbuild. The PR we're using to test is here.

Are you noticing anything different with Rollup? Or do you see anything missing from how we are bundling in our tests?

timostamm commented 4 months ago

Let's close this. See https://github.com/connectrpc/examples-es/pull/1499#issuecomment-2098596500 for details.