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

[Redirect] don't import entire history module #6278

Merged
merged 5 commits into from
Aug 5, 2018

Conversation

petermikitsh
Copy link
Contributor

Hello, I was looking through my webpack bundle and found this optimization to reduce bundle size.

With these changes, my bundle goes from 797.3kb to 782.9kb, a savings of 14.4kb.

Before:

screen shot 2018-08-02 at 4 24 14 pm

After:

screen shot 2018-08-02 at 4 24 22 pm

@petermikitsh
Copy link
Contributor Author

I've also updated the jest config so the unit test failures pass. These unit tests were failing prior to this PR -- they are not a regression introduced by this PR.

@timdorr
Copy link
Member

timdorr commented Aug 5, 2018

Thanks!

@timdorr timdorr merged commit 13a855b into remix-run:master Aug 5, 2018
@billyjanitsch
Copy link
Contributor

@timdorr Are you sure about this one? The current import style was chosen intentionally in #5589.

This new PR prevents webpack from being pointed at the ESM build of history, and since react-router now contains some imports from 'history' (which will correctly point at the ESM build) and some from 'history/LocationUtils' (which will not), an ESM build will actually contain LocationUtils and its imports twice, once as ESM and once as CJS.

A better fix would be to add {sideEffects: false} in the history module's package.json, for which there is an open PR: remix-run/history#597. This would provide the best of both worlds.

Please consider reverting this before releasing a new version of react-router. 🙂

jeresig pushed a commit to Khan/react-router that referenced this pull request Aug 29, 2018
* [Redirect] don't import entire history module

* fix jest config (jestjs/jest#6769)

* fix jest config (jestjs/jest#6769)

* fix jest config (jestjs/jest#6769)

* fix jest config (jestjs/jest#6769)
@lock lock bot locked as resolved and limited conversation to collaborators Oct 9, 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.

3 participants