Skip to content
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

Support for finer grained tree shaking #296

Open
alehechka opened this issue Oct 26, 2022 · 7 comments
Open

Support for finer grained tree shaking #296

alehechka opened this issue Oct 26, 2022 · 7 comments
Labels
enhancement New feature or request

Comments

@alehechka
Copy link

Is your feature request related to a problem? Please describe.
When using Vite to produce the production build of a React app, it was found that tree-shaking only occurs per file instead of per function within each file.

In my repository, I have two different proto files, each with its own service. When using the generator for connect-web, it results in respective TS files for each proto file. Currently, I have not imported any code from one of the proto files, and only one method from the other. After building the production build with Vite and inspecting the bundle, there were no methods from the unused proto definition (as expected). However, all methods from the proto definition, in which I'm only using one method, were included in the bundle.

In my case, the service that I am only using one method of in the frontend, I am using the others for inter-service communication in the backend. Those other methods are not accessible over the internet, but it is still a concern that all the details of those methods are sent in the JS bundle.

To get around this, I am breaking off the service methods that are for the frontend into their own proto definition file where the production build will only include these methods in the final bundle.

Describe the solution you'd like
Have the generated connect-web TS code be better accessible for fine-grained tree shaking at a per function/method level instead of entire files.

Describe alternatives you've considered
As mentioned earlier, I am breaking the methods I want to use in the frontend into their own service and proto definition file where they will be built into their own TS files in the package and thus everything will be tree shaken out.

@alehechka alehechka added the enhancement New feature or request label Oct 26, 2022
@timostamm
Copy link
Member

Adam, thanks for raising this, we've been pondering it as well. For most effective tree-shaking, it would be ideal to export a symbol for each individual RPC. On the other hand, a service containing a set of methods is the most natural mapping from protobuf sources, so we went with that.

There is also the option to use async imports - types would still be available at build time, but the actual message definitions could be split into a separate bundle that is loaded on demand at run time.

@alehechka
Copy link
Author

Thanks for the quick response! The reasoning you gave makes sense. I do have one question however.

When testing different setups with the proto definition to figure out the line for tree shaking, I did test having two different services with their own methods defined within one proto file. The generated TS code then had both services defined with the one set of TS files with the respective file name. However, even with two different services defined in the same TS file, and only one getting imported and used in the client-side code, both services and their methods were bundled into the production build.

I put together a simple example repo with slimmed down proto definitions with what I mean here: https://github.com/alehechka/connect-web-tree-shaking

Under /proto/api/example/v1 you can see there are two different proto files, each with their own messages and services. The example.proto contains two different services each with their own messages for request/response.

The code is then generated under /src/proto/gen/ts/example/v1.

Finally, within /src/App.tsx, I am importing and using exclusively the ExampleService.

However, after running the production vite build, both the ExampleService and HelloService are included in the bundle (available at /dist/assets/index.b2b3071a.js)

@timostamm
Copy link
Member

For your example.proto, we generate two declarations:

export const ExampleService = { ... };
export const HelloService = { ... };

Let's say you bundle your application from an entry point index.ts:

import {ExampleService} from "example_connectweb.js";
console.log(ExampleService);

Any sensible implementation of a tree-shaker will eliminate HelloService from the bundle, and it is indeed not present in the bundle you linked to.

I do see HelloRequest and HelloResponse in the bundle, and it is not clear to me why that would be the case. We have the following import statement in example_connectweb.ts:

import {ExampleRequest, ExampleResponse, HelloRequest, HelloResponse} from "./example_pb.js";

I suspect that the tree shaker is overly cautious here for an unknown reason. It would be really cool to analyze this a bit (splitting up the import statement into 4 individual ones, for a start), and clarify with the tree shaker authors why HelloRequest and HelloResponse are not elided, and whether this could be improved in the shaking algorithm. Perhaps this is something you'd like to investigate?

@alehechka
Copy link
Author

Ah, I see now that HelloService was removed, just the HelloRequest and HelloResponse messages included.

I did some quick tests with the separated imports:

import { ExampleRequest } from './example_pb.js';
import { ExampleResponse } from './example_pb.js';
import { HelloRequest } from './example_pb.js';
import { HelloResponse } from './example_pb.js';

Even with them separated, all four are still included while only using the ExampleService. I then dug deeper by changing the HelloService to not use HelloRequest and HelloResponse and remove their imports. Unfortunately, the messages were still included in the bundle.

It wasn't until I also removed usage of ExampleRequest and ExampleResponse in the ExampleService that it was finally able to tree shake all of these messages out of the bundle.

This is very curious, because like you said, these messages are all individually exported and should be easily tree shaken if they are not used. If I have some time, I'll try and look into the tree shaking algorithm or clarify with the authors what might be happening here.

@mckelveygreg
Copy link

I'm also running into this issue. We end up with a 152kb package that just handles our generated connect-web code 😬 I think I may put it in a manual chunk until we figure out something better 🤷

@timostamm
Copy link
Member

@mckelveygreg, if you are generating to a package, make sure to set "sideEffects": false.

Depending on your set of protobuf files, it may also pay off to generate .js + .d.ts instead of .ts - for example, for example.proto, this will only generate the bare minimum of data needed to parse a message.

Also note that even gzip is pretty good at reducing generated code to a small fraction of the uncompressed bundle.

@timostamm
Copy link
Member

@mckelveygreg, there is also another option that may or may not be useful, depending on your use case: v1.10.0 of the Buf CLI added a flag that basically adds tree-shaking before code generation. For example, the following command generates only the rpc Say and the messages required for it:

buf generate buf.build/bufbuild/eliza --include-types buf.connect.demo.eliza.v1.ElizaService.Say

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants