Skip to content

Do not bundle dependencies in cjs/esm builds #53

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

Merged
merged 1 commit into from
Jun 12, 2018

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Apr 10, 2018

cjs build - minified & gzipped

pre - 5316 bytes
post - 2069 bytes

size reduction - 61% 💰

@banderson
Copy link

🙏

@Andarist
Copy link
Contributor Author

@mjackson friendly 🏓

@mjackson
Copy link
Member

mjackson commented Jun 3, 2018

Thanks, @Andarist. This means that people need to already have React and ReactDOM on the page, right?

@Andarist
Copy link
Contributor Author

Andarist commented Jun 3, 2018

This PR introduces external management with process.env.EXTERNALS, the env var is only used for both prod & dev UMD builds with "peers" value and yes that means users have to have React on the page (react-broadcast does not depend anyhow on ReactDOM), but that is not different from the situation before this PR. It was already kept as external, just the value was statically passed into the config. Also this is the way to go, we do not want each library depending on React having to have its own copy.

The most important part of this is that without using explicit process.env.EXTERNALS the external array defaults to deps.concat(peers) which kicks in for both CJS & ESM bundles. Those are primarily used by bundlers / node and we shouldn't bundle deps into those bundles because then bundlers cannot dedupe this code, each such bundle keeps its own copy of those deps which increases the final bundle size.

@mjackson
Copy link
Member

Ah, I think I see now. So when we are building the UMD bundle we say that only peers are external, but when we are building CJS or ESM we say that both peers + other deps are external, so the bundlers don't include stuff twice. I think that makes sense.

Thanks for taking such care here, @Andarist. The solution you came up with is actually quite nice. I'll get this merged and cut a new release.

@mjackson mjackson merged commit 8d1575e into ReactTraining:master Jun 12, 2018
@mjackson
Copy link
Member

Published in 0.7.1

Thanks again!

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

Successfully merging this pull request may close these issues.

3 participants