Skip to content

Conversation

@BinaryMuse
Copy link
Contributor

Teams at GitHub saw a significant increase in their built bundle sizes upgrading from PC v18 to the v19 release candidate built from #764. Upon investigation, it appears webpack is including the entirety of the PC lib in their bundle, instead of tree-shaking like it was in the past.

This is probably due to the changes I made to the Babel compilation; in particular, in development, it transpiles the src/ files into CJS modules.

@vercel
Copy link

vercel bot commented May 7, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/primer-components/5iriuaip4
✅ Preview: https://primer-components-git-mkt-tweak-babel-transpilation.primer.now.sh

@vercel vercel bot temporarily deployed to Preview May 7, 2020 16:55 Inactive
@BinaryMuse
Copy link
Contributor Author

BinaryMuse commented May 7, 2020

Okay, here are my findings:

PC Version Total Chunk Size PC Size PC Percent Notes
v18.0.0 366.4 KB 195 KB 53.1% 161ms
v19.0.0-rc 413.8 KB 219 KB + ? 52.9% + ? includes all of PC and all deps (difficult to measure total size)
canary w/ ESM lib folder 364.0 KB 196 KB 53.9% back to baseline size
canary w/ ESM browser bundle 304.9 KB 122 KB 40% chunk size down 60 KB!
canary w/ ESM dev bundle 305.9 KB 153 KB 50%

The first row is the baseline chunk with v18, and the second row is the v19-rc. The bundle size grew 47.4 KB, even though the PC source didn't change size, because by default PC points to the CJS files and tree-shaking no longer works.

In the third row, I changed Babel so that it didn't touch the module format, making all the files in lib/ (which main points to) ESM compatible modules. This got us back down to the baseline (actually shaved off a couple KB).

In the next-to-last row, I decided to keep compiling lib/ as CJS and just set package.json's module field to dist/browser.esm.js, the ESM browser build. This dropped the final bundle size to 304.9 KB, a 60 KB savings! Finally, the last row uses the dev build of the ESM bundle, but while the PC bundle size grew by ~30KB, the final bundle size didn't change much (probably because the CRA build script uses NODE_ENV=production as well).

My gut is to leave it this way assuming it doesn't cause any issues. If you're using the ESM build, you don't need to be able to import components individually because tree-shaking takes care of it for you; the individual built files in lib/ are only useful to people using a CJS setup, so I feel it makes sense to continue to compile these to CJS.

What do you all think?

@vercel vercel bot temporarily deployed to Preview May 7, 2020 17:37 Inactive
@vercel vercel bot temporarily deployed to Preview May 7, 2020 17:53 Inactive
@vercel vercel bot temporarily deployed to Preview May 7, 2020 18:12 Inactive
@vercel vercel bot temporarily deployed to Preview May 7, 2020 18:46 Inactive
@vercel vercel bot temporarily deployed to Preview May 7, 2020 20:44 Inactive
@BinaryMuse
Copy link
Contributor Author

BinaryMuse commented May 7, 2020

After a bunch of back-and-forth with teams using this canary in a real-world app, I've decided to remove all the unknown variables and do the simplest thing: transpile a copy of the source separately for ESM users and CJS users. So here's the way the package would look after fetching it from npm:

  • dist/browser.{esm,umd}.js — full ESM and UMD bundles, built in production mode with all dependencies, as before
  • lib/**/*.js — raw source transpiled to CJS format
  • lib-esm/**/*.js — raw source transpiled to ESM format; the package.json module field points to this directory

@vercel vercel bot temporarily deployed to Preview May 7, 2020 21:00 Inactive
@BinaryMuse BinaryMuse marked this pull request as ready for review May 7, 2020 21:03
@BinaryMuse BinaryMuse merged commit 0975930 into release-19.0.0 May 7, 2020
@BinaryMuse BinaryMuse deleted the mkt/tweak-babel-transpilation branch May 7, 2020 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants