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

fix: add sideEffects: false to react-error-overlay #5451

Merged
merged 1 commit into from
Nov 22, 2018

Conversation

SimenB
Copy link
Contributor

@SimenB SimenB commented Oct 16, 2018

This allows us to leave the import in the code, and webpack will still tree shake it out.

Verified by grepping for startReportingRuntimeErrors in the resulting bundle.

Note that this is usage outside of react-dev-utils.

Without this change, I need to use require and not import, and stick my require inside of an environment check.

Docs: https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free

@stale
Copy link

stale bot commented Nov 15, 2018

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Nov 15, 2018
@stale
Copy link

stale bot commented Nov 20, 2018

This pull request has been automatically closed because it has not had any recent activity. If you have a question or comment, please open a new issue. Thank you for your contribution!

@stale stale bot closed this Nov 20, 2018
@SimenB
Copy link
Contributor Author

SimenB commented Nov 20, 2018

Why have a bot autoclose PRs that hasn't been acknowledged by the maintainers? Not the friendliest thing in the world (I personally don't really care, but it can be disheartening to beginners, methinks)

/cc @Timer mind taking a look at this?

@Timer
Copy link
Contributor

Timer commented Nov 20, 2018

Can you please confirm the bundle is byte-for-byte identical when this flag is enabled (well... disabled -- false) and the overlay is imported & used?

@Timer Timer reopened this Nov 20, 2018
@stale stale bot removed the stale label Nov 20, 2018
@Timer Timer added this to the 2.1.x milestone Nov 20, 2018
@SimenB
Copy link
Contributor Author

SimenB commented Nov 20, 2018

What bundle do you refer to? An app built with CRA, or the built version of just the overlay?

@Timer
Copy link
Contributor

Timer commented Nov 20, 2018

The dev bundle.

This allows us to leave the import in the code, and webpack will still tree shake it out
@SimenB
Copy link
Contributor Author

SimenB commented Nov 20, 2018

Yes, they're identical 🙂

$ yarn create react-app myapp
$ cd myapp
$ yarn start &
$ curl -s http://localhost:3000/static/js/bundle.js | md5
c2e99be9db96e20f22b9bd578cc80fea
$ curl -s http://localhost:3000/static/js/0.chunk.js | md5
07e94a964f183f6edaee07cb113c1e03
$ curl -s http://localhost:3000/static/js/main.chunk.js | md5
09ec1827050bed667003a85530d36ac4

# Change package.json of error overlay, and restart server

$ curl -s http://localhost:3000/static/js/bundle.js | md5
c2e99be9db96e20f22b9bd578cc80fea
$ curl -s http://localhost:3000/static/js/0.chunk.js | md5
07e94a964f183f6edaee07cb113c1e03
$ curl -s http://localhost:3000/static/js/main.chunk.js | md5
09ec1827050bed667003a85530d36ac4

@Timer Timer modified the milestones: 2.1.x, 2.1.2 Nov 20, 2018
@Timer
Copy link
Contributor

Timer commented Nov 22, 2018

can't get CI to pass, YOLO

@Timer Timer merged commit 2c92fd4 into facebook:master Nov 22, 2018
@SimenB SimenB deleted the patch-1 branch November 22, 2018 05:51
dardub added a commit to OffBase/create-react-app that referenced this pull request Nov 27, 2018
* upstream/master: (210 commits)
  Support setupTests.ts (facebook#5698)
  Remove unnecessary whitespace in template HTML
  Run prettier on HTML files (facebook#5839)
  Some Grammar fixes (facebook#5858)
  Fix link to page about running tests (facebook#5883)
  fix: make typescriptformatter support 0.5 of fork checker (facebook#5879)
  Always test with the latest stable Node version on Travis (facebook#5546)
  Fix propertyDecorator test
  Upgrade babel deps
  Fix annotated var test
  Fix TypeScript decorator support (facebook#5783)
  fix: add `sideEffects: false` to react-error-overlay (facebook#5451)
  Add allowESModules option to babel-preset-react-app (facebook#5487)
  Make named-asset-import plugin work with export-as syntax (facebook#5573)
  React native repository updated in README.md (facebook#5849)
  extra polyfills must be included manually (facebook#5814)
  Rename 'getting started' link to 'docs' (facebook#5806)
  docs: Simplify installing Storybook with npx (facebook#5788)
  Don't polyfill fetch for Node -- additional files (facebook#5789)
  docs: Change Storybook install documentation (facebook#5779)
  ...
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants