refactor(build): update rollup build step #2257
Conversation
|
size-limit report 📦
|
| rm -rf dist | ||
| rm -rf lib | ||
| rm -rf lib-esm | ||
| npm run clean |
| const external = [ | ||
| ...Object.keys(packageJson.peerDependencies), | ||
| ...Object.keys(packageJson.dependencies), | ||
| ...Object.keys(packageJson.devDependencies) |
There was a problem hiding this comment.
Is this how the bundle size is reduced?
|
I’ll let other folks weigh in here, but thought this might be prudent to share (x-posting from Slack for posterity) -- #web-systems, our sister UI platform team over in the Eng org, historically made the call to use rollup to build GitHub assets. More recently, they decided to roll back this decision and adopt webpack instead. Perhaps there’s insight about their decision-making process in here that could be useful for us to consider, too. |
pksjce
left a comment
pksjce
left a comment
There was a problem hiding this comment.
This is a really cool change! I have a few questions.
- Will the lib-esm package size decrease be felt by our consumers? Will their bundle sizes decrease?
- What kind of tests were you able to run for these new bundled files? Can you make sure nextjs/remix are able to consume them with no issues? We've had tons of issues regarding bundling now working for these frameworks. Maybe you could test in memex too?
- Did you test for importing draft components. The subpaths were one of my pain points with esm modules.
- Also I'm a little apprehensive about making dependencies external. Maybe I don't understand the context properly.
| extensions | ||
| }), | ||
| commonjs(), | ||
| babel({ |
There was a problem hiding this comment.
I'd recommend avoiding babel here. We dropped babel from dotcom and all public repos a few years ago because:
- babel is quite slow. Typescript or esbuild or swc are much faster and equally as capable
- Babel tends to err on the side of compatibility which means transforming things that don't necessarily need to be transformed due to edge cases. (An example of this is object rest spread gets transformed if you specify edgehtml as a browser, even though edge supports object rest spread). This results in bigger bundle sizes than necessary.
- Configuring babel can be tricky and can result in varying compatibilities. Typescripts
targetis a little more blunt (just specifying a year to compile down to) but that's much easier to propagate across many repos.
In addition, moving from babel would be a great step to coalescing all our repositories around a central build config. As mentioned doctor uses swc, while all other repositories just use typescript.
Here's Primer ViewComponents configuration: https://github.com/primer/view_components/blob/b0dbefb471d8262a2a45235ccaf0501260f00cd4/rollup.config.js#L15
This makes sense given the context of dotcom which requires delivering of assets in ways which webpack supports better than rollup, but for libraries we still use rollup across the board as it provides much simpler configuration. |
|
Thanks for taking the time to review @pksjce!
Sadly not much, I think the only thing would be that Babel currently is transpiling files 1:1 whereas rollup is able to build a dependency graph and extract common helpers. But that isn't too much savings in the grand scheme of things, especially since the project already is setup for tree shaking.
Nothing yet, wanted to gauge interest first since this change would require updating the circular dependencies in the project first 👀 I love all your suggestions and am happy to test them out and keep this exploration going!
Thankfully
The way I understand this is that rollup by default will include all dependencies in the output bundle. This is great for UMD bundles but in the ESM/CommonJS case it's nice to exclude them because they can be resolved via |
|
Thanks all for leaving a comment! Going to close out this exploration and will post a follow-up after addressing the circular deps issue 😄 Will also make sure to revisit the rollup setup with TypeScript vs Babel 👍 |
This is a draft PR to see what it would be like to use
rollupfor the full build process. Using rollup in this way can:It can also be a useful tool for highlighting when modules have circular dependencies within the project. It also has the side-effect of only production artifacts making their way into the package (no tests or stories). Types still seem to be emitted by the build process.
Package size (on disk)
lib-esmlibThis PR also marginally decreases the build time from roughly 18s to 13s, mostly due to it using the same bundle to output cjs/esm/umd instead of building once in rollup and compiling once for cjs/esm.
time npm run buildBlockers
This change cannot be adopted until circular dependencies are addressed