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

Bundle size is unnecessarily big (open discussions, infinite corridor) #3714

Closed
ChristopherChudzicki opened this issue Aug 31, 2022 · 4 comments
Labels

Comments

@ChristopherChudzicki
Copy link
Contributor

The bundle sizes for infinite-corridor and open-discussions have both jumped considerably recently. For example, [@fakerjs/faker](https://www.npmjs.com/package/@faker-js/faker) is bloating both bundles by 2.6M even though we only use it for tests.

@ChristopherChudzicki
Copy link
Contributor Author

ChristopherChudzicki commented Aug 31, 2022

I looked into this a bit. In general, I think the local modules, e.g., ol-search-ui should probably not contain top-level exports that we only use for testing. (Which they currently do, namely the factory functions for creating test data.) So I think we should set up the exports to be like

// used in app
import { LearningResourceCard } from "ol-search-ui"
// only used for tests
import { makeLearningResource} from "ol-search-ui/test-utils"

whereas currently both are top-level exports from "ol-search-ui".

It should be possible to keep both as top-level exports and tree shake the testing-related exports out during the prod build, but it can get a little complicated and depends exactly how the dependencies are written. If an import is seemingly unused but includes top-level function calls (e.g., a react HOC) webpack won't automatically treeshake it because the function call could have side effects. In scenarios like that, the module authors have to explicitly mark the module as side-effect free. There's an open issue in the faker repo to do that for faker: faker-js/faker#1305

@Shinigami92
Copy link

I had a look into your repo via https://github.com/mitodl/open-discussions/search?p=1&q=faker
Looks like lorem is the only API that calls localized values, but even there you take the default faker instance without changing the locale to non-"en"
So I would highly recommend for now to use import { faker } from "@faker-js/faker/locale/en" to reduce the faker instance from around 4mb to 600kb

For later things please subscribe to: faker-js/faker#1340 and faker-js/faker#1351
You might be specially interested in the BaseFaker we want to introduce

@ChristopherChudzicki
Copy link
Contributor Author

@Shinigami92 Thanks for taking a look, and your work on faker! Interested to follow those issue, especially new module structure. Since it's a dependency we only use in development, the size of faker doesn't really worry me, it's the fact that it was included in our production build at all. (Which is easy for us to avoid with some simple changes to our local modules.)

@ChristopherChudzicki
Copy link
Contributor Author

Closing this for now ; #3725 did a lot. More could be done, of course.

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

No branches or pull requests

2 participants