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

build: improve tree shaking #3803

Merged
merged 6 commits into from Dec 2, 2021
Merged

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented Nov 26, 2021

What's the problem this PR addresses?

  • Some of the JS code generated by Emscripten contains browser specific code that Rollup can't tree shake.
  • Rollup is preserving code that could possibly have side effects but we know doesn't.
  • Rollup generates es5 code.

How did you fix it?

  • Define a few browser globals as undefined so Rollup can remove more unused code.
  • Enable more aggressive tree shaking.
  • Update Rollup and configure it to generate smaller es2015 code.

Result

Tested on this repo

$ du -sb ./.pnp-*
2665838 ./.pnp-after.cjs
2670400 ./.pnp-before.cjs

Checklist

  • I have read the Contributing Guide.
  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

Comment on lines 12 to 16
treeshake: {
propertyReadSideEffects: false,
unknownGlobalSideEffects: false,
tryCatchDeoptimization: false,
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about those, as it would be a massive pain to debug any issue they could accidentally cause. The gain seems fairly low, so I'd tend to favour long-term maintainability.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair, reverted 👍

@merceyz merceyz force-pushed the merceyz/chore/improve-tree-shaking branch from 5c40c94 to c5d2f9d Compare December 1, 2021 13:45
@arcanis
Copy link
Member

arcanis commented Dec 1, 2021

Tests show something broke

@merceyz
Copy link
Member Author

merceyz commented Dec 1, 2021

Seems to be rollup/rollup#4195, fixed by updating @rollup/plugin-commonjs

@arcanis arcanis merged commit dceb8fc into master Dec 2, 2021
@arcanis arcanis deleted the merceyz/chore/improve-tree-shaking branch December 2, 2021 13:27
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.

None yet

2 participants