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

Add sideEffects: false to react-router-dom package.json #6082

Merged
merged 3 commits into from
Apr 11, 2018

Conversation

taylorc93
Copy link
Contributor

Webpack 4.0 introduced support for a sideEffects flag in a project's package.json: https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free. This is awesome because we can now get Webpack's tree shaking to work as intended for most, if not all projects. 🎉

I've noticed that in my projects where I've used react-router-dom, the recommended import syntax (eg. import { Route } from 'react-router-dom') still pulls in the entire library. While react-router-dom and it's associated packages aren't that large, I'm a bit of a performance junkie 😃. All this PR does is add the sideEffects: false flag to the react-router-dom package. I created a simple test repo to demo the effects of this flag: https://github.com/taylorc93/react-router-test. I'm noticing ~20KB of minified JS removed from my bundle as a result of this change.

The only thing I'm slightly unsure of is whether there are, in fact, side effects. From what I can see, there aren't any, but I could definitely be missing something. Let me know what you think / if there's anything else you need from me to get this merged!

Side note: Love this project, I've been using it for years and it's worked fabulously. Keep up the great work!

@pshrmn pshrmn changed the title added sideEffects: true to react-router-dom Add sideEffects: false to react-router-dom package.json Apr 10, 2018
@timdorr
Copy link
Member

timdorr commented Apr 11, 2018

Can we add this to the other packages as well? I put it on Redux as well. It doesn't really hurt, since we don't modify globals.

@taylorc93
Copy link
Contributor Author

@timdorr All set

@timdorr
Copy link
Member

timdorr commented Apr 11, 2018

Thanks!

@timdorr timdorr merged commit 7ae8cf5 into remix-run:master Apr 11, 2018
@sorrycc sorrycc mentioned this pull request Apr 26, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jun 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants