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

Make the library more tree-shakable to significantly reduce bundle size #514

Open
adamstoffel opened this issue Feb 11, 2025 · 5 comments
Open
Assignees
Labels
enhancement New feature or request

Comments

@adamstoffel
Copy link

adamstoffel commented Feb 11, 2025

Is your feature request related to a problem? Please describe.

Because this library is implemented as one massive exported BoxClient class, it is impossible for bundlers like webpack and rollup to perform effective tree-shaking and remove unused portions of library code. In our app we are using this SDK in a client-side browser context, so bundle size is important to us, and the Box SDK comprises nearly half of our client-side JS bundle. (Of note, the hash-wasm package, which the Box SDK depends on, is also a very large package that could be eliminate for folks who don't need uploads.)

Image

Describe the solution you'd like

There's some incremental steps that could be taken to improve tree shaking (there is a huge leap from 2 to 3):

  1. Ensure sideEffects: false is set in package.json. I don't expect this to actually have a significant impact, but is always a first step.
  2. Export the different API Manager classes as separate uniquely instantiable client classes. This would provide the immediate benefit of tree shaking tools being able to eliminate unused code from any API which is not being called. This could probably be implemented without any breaking changes for current consumers of BoxClient. Documentation should also be updated to prefer this syntax.
  3. Ultimately maximum tree-shaking capability would require exporting individual API methods as functions, perhaps accepting common configuration options as a first or last function parameter or using an object parameter pattern. This would be a significant re-write and new pattern for the library.

Describe alternatives you've considered

None?

Additional context

Additional references:

@mwwoda
Copy link
Contributor

mwwoda commented Feb 11, 2025

Hello,

I believe our Managers should act similarly as described in the second point. The BoxClient just aggregates them. You still should be able to use them on their own like this

import { UsersManager } from "box-typescript-sdk-gen/lib/managers/users.generated";
import { NetworkSession } from "box-typescript-sdk-gen/lib/networking/network.generated";
import { UserFull } from "box-typescript-sdk-gen/lib/schemas/userFull.generated";
import { BoxDeveloperTokenAuth } from "box-typescript-sdk-gen/lib/box/developerTokenAuth.generated";

const auth = new BoxDeveloperTokenAuth({
token: "",
});

const networkSession = new NetworkSession({});

const usersManager = new UsersManager({
auth: auth,
networkSession: networkSession,
});

const response = await usersManager.getUserMe();

As for the tree shaking, I see that there are some issues with barrel file we expose if you import code from it (we re-export BoxClient there).

You could try to adjust the imports to import files directly

Before
import { BoxDeveloperTokenAuth } from "box-typescript-sdk-gen";

After:
import { BoxDeveloperTokenAuth } from "box-typescript-sdk-gen/lib/box/developerTokenAuth.generated";

@adamstoffel
Copy link
Author

You're totally right - instantiating the managers we want directly (and importing from the *.generated location) does enable some tree-shaking and reduce size quite a bit.

In this case, I guess it would be convenient to:

  1. Have those classes be importable in a more succinct way
  2. Not have to pass a networkSession object to every manager constructor

Here's what our bundle looks like when we only import the managers we need (file, folders, uploads, chunkedUploads, downloads, and zipDownloads):

Image

@mwwoda
Copy link
Contributor

mwwoda commented Feb 12, 2025

Cool to see such reduction in bundle size.

As for your suggestions

  1. It is definitely worth investigating whether this can be improved without falling into the barrel files trap again.
  2. it seems to me that the assumption was that BoxClient would always pass networkSession and the fact that Manager cannot work without it. We would need to explore whether we could make it an optional argument with a default value.

@adamstoffel
Copy link
Author

adamstoffel commented Feb 13, 2025

Another big improvement could come from building and distributing an ESM version of the library. Since the library is currently compiled to CommonJS, there's no ability for tree-shaking to eliminate dead code in downstream dependencies.

Namely, the I realized the entire hash-wasm library (with all algorithms) is being included in my bundle despite the fact that hash-wasm itself is distributed as ESM and is tree-shakeable. That's contributing ~215K to my bundle when it should be closer to 20K for just HMAC, SHA1, and SHA256.

I'm quite certain this is because lib/internal/utils.ts transpiles to include hash-wasm using CJS require syntax, resulting in the entire library being included:

const hash_wasm_1 = require("hash-wasm");

ESBuild is not able to tree-shake CJS-style require() imports.

@adamstoffel
Copy link
Author

adamstoffel commented Feb 14, 2025

Would you be open to me submitting a PR which adds the changes necessary to generate and publish this as a hybrid UMD & ES package? I was able to do this with a local copy of the library and, for my case, reduced the Box assets even further from 448K to 388K and hash-wasm from 215K to 13K!

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

6 participants