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

Throw error to warn of mistaken loading of prod + dev React bundles #10446

Merged
merged 11 commits into from Aug 16, 2017

Conversation

Projects
None yet
5 participants
@flarnie
Contributor

flarnie commented Aug 11, 2017

what is the change?:
Adds an error indicating a gotcha where users end up loading both the 'prod' and 'dev' bundles in production.

Credit to @gaearon for coming up with a clever way to check for this. :)

I mainly just did the manual testing and fixed a couple of typos in his
idea.

I'd like to do a follow-up PR to add a link and a page explaining this
issue more and suggesting how to fix it. Would folks support that?

why make this change?:

We want to warn for an unfortunate gotcha for
the following situation -

  1. Wanting to shrink their JS bundle, an engineer runs it through
    'uglifyjs' or some other minifier. They assume this will also do
    dead-code-elimination, but the 'prod' and 'dev' checks are not
    envified yet and dev-only code does not get removed.
  2. Then they run it through browserify's 'envify' or some other tool to
    change all calls to 'process.env.NODE_ENV' to "production". This
    makes their code ready to ship, except the 'dev' only dead code is
    still in the bundle. Their bundle is twice as large as it needs to
    be, due to the dead code.

This was a problem with the old build system before, but with our new
build system output it's possible to detect and throw an informative
error in this case.

test plan:

  1. run the build in react as usual; yarn build
  2. manually run 'uglifyjs -mt' on 'build/packages/react/index.js' to
    simulate mistakenly minifying React before env variables are
    resolved, which skips dead code elimination.
  3. run the fixtures build - cd fixtures/packaging && node ./build-all.js && serve ../..
  4. Visit just the production browserify fixture -
    http://localhost:5000/fixtures/packaging/browserify/prod/
  5. You should see the error thrown indicating this problem has occurred.

screen shot 2017-08-11 at 4 07 44 pm

6. Do the above steps with NO uglifyjs step, and verify that no error is thrown. When there is no minification applied, we don't assume that this mix-up has occurred.

screen shot 2017-08-11 at 4 10 56 pm

issue:
fixes #9589

Throw error to warn of mistaken loading of prod + dev React bundles
**what is the change?:**
Credit to @gaearon for coming up with a clever way to check for this. :)

I mainly just did the manual testing and fixed a couple of typos in his
idea.

I'd like to do a follow-up PR to add a link and a page explaining this
issue more and suggesting how to fix it.

**why make this change?:**

We want to warn for an unfortunate gotcha for
the following situation -
1. Wanting to shrink their JS bundle, an engineer runs it through
   'uglifyjs' or some other minifier. They assume this will also do
   dead-code-elimination, but the 'prod' and 'dev' checks are not
   envified yet and dev-only code does not get removed.
2. Then they run it through browserify's 'envify' or some other tool to
   change all calls to 'process.env.NODE_ENV' to "production". This
   makes their code ready to ship, except the 'dev' only dead code is
   still in the bundle. Their bundle is twice as large as it needs to
   be, due to the dead code.

This was a problem with the old build system before, but with our new
build system output it's possible to detect and throw an informative
error in this case.

**test plan:**
1. run the build in react as usual; `yarn build`
2. manually run 'uglifyjs -mt' on 'build/packages/react/index.js' to
   simulate mistakenly minifying React before env variables are
   resolved, which skips dead code elimination.
3. run the fixtures build - `cd fixtures/packaging && node
   ./build-all.js && serve ../..`
4. Visit just the production browserify fixture -
   http://localhost:5000/fixtures/packaging/browserify/prod/
5. You should see the error thrown indicating this problem has occurred.
(Flarnie will insert a screenshot)
6. Do the above steps with NO uglifyjs step, and verify that no error is
   thrown. When there is no minification applied, we don't assume that
   this mix-up has occurred.
(Flarnie will insert a screenshot)

**issue:**
fixes #9589

@flarnie flarnie added this to the 16.0 milestone Aug 11, 2017

@flarnie flarnie requested review from sophiebits, bvaughn and sebmarkbage Aug 11, 2017

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Aug 11, 2017

Member

Should we create a specific fixture for this case, where yarn build minifies without DCE as described in step 2? Would make it easier to test in the future.

Member

acdlite commented Aug 11, 2017

Should we create a specific fixture for this case, where yarn build minifies without DCE as described in step 2? Would make it easier to test in the future.

@flarnie

This comment has been minimized.

Show comment
Hide comment
@flarnie

flarnie Aug 12, 2017

Contributor

Should we create a specific fixture for this case, where yarn build minifies without DCE as described in step 2? Would make it easier to test in the future.

I like that, and we could even point to that if we add docs about this issue in the future. Would rather merge this and add that as a follow-up, so that it unblocks the RC. But I'll work on adding a fixture over the weekend if I can.

Contributor

flarnie commented Aug 12, 2017

Should we create a specific fixture for this case, where yarn build minifies without DCE as described in step 2? Would make it easier to test in the future.

I like that, and we could even point to that if we add docs about this issue in the future. Would rather merge this and add that as a follow-up, so that it unblocks the RC. But I'll work on adding a fixture over the weekend if I can.

Show outdated Hide outdated packages/react/index.js
Show outdated Hide outdated packages/react/index.js
Remove extra 'prod' env. check and add link to docs in error message
**what is the change?:**
Based on helpful feedback from @gaearon
- removed outer check that `process.env.NODE_ENV` is "production" since
  we are only calling the `testMinification` method inside of another
  check for "production" environment.
- Added an fburl that points to [our current docs on using the production version of React](https://facebook.github.io/react/docs/optimizing-performance.html#use-the-production-build)

**why make this change?:**
To save an extra layer of conditionals and to make the error message
more clear.

**test plan:**
Same test plan as earlier -

1. run the build in react as usual; yarn build
2. manually run 'uglifyjs -mt' on 'build/packages/react/index.js' to
   simulate mistakenly minifying React before env variables are
   resolved, which skips dead code elimination.
3. run the fixtures build - cd fixtures/packaging && node ./build-all.js && serve ../..
4. Visit just the production browserify fixture -
   http://localhost:5000/fixtures/packaging/browserify/prod/
   You should see the error thrown indicating this problem has occurred.
   (Flarnie will insert a screenshot in comments on the PR)
6. Do the above steps with NO uglifyjs step, and verify that no error is thrown. When there is no minification applied, we don't assume that this mix-up has occurred.
(Flarnie will insert a screenshot in the comments on the PR.)

**issue:**
#9589
@flarnie

This comment has been minimized.

Show comment
Hide comment
@flarnie

flarnie Aug 12, 2017

Contributor

Re-tested manually:

With the uglify step run before envify:

screen shot 2017-08-12 at 1 50 19 pm

With no uglify step:

screen shot 2017-08-12 at 1 56 28 pm

I'd like to land this before Monday AM if possible, and will work on adding a fixture to reproduce/test it after landing.

Contributor

flarnie commented Aug 12, 2017

Re-tested manually:

With the uglify step run before envify:

screen shot 2017-08-12 at 1 50 19 pm

With no uglify step:

screen shot 2017-08-12 at 1 56 28 pm

I'd like to land this before Monday AM if possible, and will work on adding a fixture to reproduce/test it after landing.

@gaearon gaearon referenced this pull request Aug 13, 2017

Closed

React 16 Umbrella ☂️ (and 15.5) #8854

117 of 120 tasks complete
@flarnie

This comment has been minimized.

Show comment
Hide comment
@flarnie

flarnie Aug 13, 2017

Contributor

I'm thinking of pulling testMinification out into a separate file and writing a unit test for it, which would allow us to keep an automated test of how this works when actually run through uglifyjs and babili.

That seems like it would add a bit of complexity to the build process - right now we don't expect this file to have access to anything except the two react bundles. I don't know if we want to maintain another file alongside the 'index.js' that gets copied over in the same way. I think it would be worthwhile to have a unit test for this logic though.

For now I'm just writing/running the unit test locally to verify this change, and if this makes sense I can figure out how to make this change to the build process.

Contributor

flarnie commented Aug 13, 2017

I'm thinking of pulling testMinification out into a separate file and writing a unit test for it, which would allow us to keep an automated test of how this works when actually run through uglifyjs and babili.

That seems like it would add a bit of complexity to the build process - right now we don't expect this file to have access to anything except the two react bundles. I don't know if we want to maintain another file alongside the 'index.js' that gets copied over in the same way. I think it would be worthwhile to have a unit test for this logic though.

For now I'm just writing/running the unit test locally to verify this change, and if this makes sense I can figure out how to make this change to the build process.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Aug 13, 2017

Member

+1 to making this a unit test

Member

acdlite commented Aug 13, 2017

+1 to making this a unit test

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Aug 13, 2017

Member

But I actually don't mind if it runs with Jest. If it's a separate command, that's fine, as long as it's automated and is part of CI.

Member

acdlite commented Aug 13, 2017

But I actually don't mind if it runs with Jest. If it's a separate command, that's fine, as long as it's automated and is part of CI.

WIP fix and test 'testMinificationUsedDCE' method
**what is the change?:**
- Instead of looking for one match when having the method inspect it's
  own source, we look for two matches, because the search itself will be
  a match.
- WIP moving this method into another file and testing it.

Next steps:
- Figure out why the babel.transform call is not actually minifying the
  code
- Add tests for uglifyjs
- Update build process so that this file can be accessed from
  `packages/react/index.js`

**why make this change?:**
- We had a bug in the 'testMinification' method, and I thought the name
  could be more clear. I fixed the code and also changed the name.
- In order to avoid other bugs and keep this code working in the future
  we'd like to add a unit test for this method. Using the npm modules
  for 'uglifyjs' and 'babel'/'babili' we should be able to actually test
  how this method will work when minified with different configurations.

**test plan:**
`yarn test`

**issue:**
#9589
@flarnie

This comment has been minimized.

Show comment
Hide comment
@flarnie

flarnie Aug 13, 2017

Contributor

Pushed the WIP test - I'm not sure what config I'm missing to get babili to minify the method, so putting this up in case someone knows. Based on the output here I think this will work though.

Contributor

flarnie commented Aug 13, 2017

Pushed the WIP test - I'm not sure what config I'm missing to get babili to minify the method, so putting this up in case someone knows. Based on the output here I think this will work though.

flarnie added some commits Aug 13, 2017

Add babeli and uglifyjs as dev-only dependencies
**what is the change?:**
See commit title

**why make this change?:**
In order to test that we are correctly detecting different minification
situations, we are using these modules in a test.

**test plan:**
NA

**issue:**
#9589
Fix typo and add 'uglifyjs' tests
**what is the change?:**
There was a mix-up in the conditional in 'testMinificationUsedDCE' and
this fixes it.

The great new tests we added caught the typo. :)

Next steps:
- get the tests for 'babili' working
- update build scripts so that this the 'testMinificationUsedDCE'
  module is available from 'packages/react/index.js'

**why make this change?:**
We want to modularize 'testMinificationUsedDCE' and test it.

This verifies that the method warns when `uglifyjs` is used to minify
but dead code elimination is not done, in a production environment.
Generally this is a 'gotcha' which we want to warn folks aboug.

**test plan:**
`yarn test src/shared/utils/__tests__/testMinificationUsedDCE-test.js`

**issue:**
#9589
@flarnie

This comment has been minimized.

Show comment
Hide comment
@flarnie

flarnie Aug 14, 2017

Contributor

I could use some context on the build process and where to start when making a change to it in order to add a new file to the build/packages/react/cjs.

Contributor

flarnie commented Aug 14, 2017

I could use some context on the build process and where to start when making a change to it in order to add a new file to the build/packages/react/cjs.

@flarnie

This comment has been minimized.

Show comment
Hide comment
@flarnie

flarnie Aug 14, 2017

Contributor

I'm also not seeing the same test failures locally that CI is showing. Will look into this more on Monday.

screen shot 2017-08-13 at 6 16 36 pm

Contributor

flarnie commented Aug 14, 2017

I'm also not seeing the same test failures locally that CI is showing. Will look into this more on Monday.

screen shot 2017-08-13 at 6 16 36 pm

flarnie added some commits Aug 14, 2017

Require specific version of `uglify-js`
**what is the change?:**
Removed the '^' from the npm package requirement

**why make this change?:**
I am seeing failures in CI that show `uglify-js` is returning different
output there from my local environment. To further debug this I'd like
to run CI with the exact same version of `uglify-js` that I am using
locally.

**test plan:**
push and see what CI does

**issue:**
#9589
Add build step copying testMinificationUsedDCE into build/packages/re…
…act/cj

**what is the change?:**
This is a first step - we still need (I think) to process this file to
get it's contents wrapped in an 'iffe'.

Added a step to the build script which copies the source file for the
'testMinificationUsedDCE' module into the 'cjs' directory of our react
package build.

**why make this change?:**
We want this module to be available to the 'index.js' module in this
build.

**test plan:**
Will do manual testing once the build stuff is fully set up.

**issue:**
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 14, 2017

Member

The last commit doesn't feel right to me. IMO we don't want to add more special cases like this that make our bundling more complicated. Especially since this particular file is not part of bundling process.

Wouldn't it be enough to add this file to packages/react/cjs (and whitelist it in files in package.json)?

Although at that point it seems like a lot of complexity just to test something. IMO this is one of those cases where we need to get the semantics right, document it, and we'll likely not need to touch it again. I don't think it's worth complicating the build process or test infra to verify this function works as expected.

Member

gaearon commented Aug 14, 2017

The last commit doesn't feel right to me. IMO we don't want to add more special cases like this that make our bundling more complicated. Especially since this particular file is not part of bundling process.

Wouldn't it be enough to add this file to packages/react/cjs (and whitelist it in files in package.json)?

Although at that point it seems like a lot of complexity just to test something. IMO this is one of those cases where we need to get the semantics right, document it, and we'll likely not need to touch it again. I don't think it's worth complicating the build process or test infra to verify this function works as expected.

@flarnie

This comment has been minimized.

Show comment
Hide comment
@flarnie

flarnie Aug 14, 2017

Contributor

Although at that point it seems like a lot of complexity just to test something.

Yea, that was what I was thinking when I commented earlier asking whether this was a good direction to go in - "That seems like it would add a bit of complexity to the build process - right now we don't expect this file to have access to anything except the two react bundles."

Writing the test in the first place helped catch another bug and validate that the current version works, so I think was worthwhile to write it, whether we decide to keep it or not.

Wouldn't it be enough to add this file to packages/react/cjs (and whitelist it in files in package.json)?

Let me see if that works, and if not then let's consider landing this without the unit test and revisiting whether it's worth adding extra complexity.

Contributor

flarnie commented Aug 14, 2017

Although at that point it seems like a lot of complexity just to test something.

Yea, that was what I was thinking when I commented earlier asking whether this was a good direction to go in - "That seems like it would add a bit of complexity to the build process - right now we don't expect this file to have access to anything except the two react bundles."

Writing the test in the first place helped catch another bug and validate that the current version works, so I think was worthwhile to write it, whether we decide to keep it or not.

Wouldn't it be enough to add this file to packages/react/cjs (and whitelist it in files in package.json)?

Let me see if that works, and if not then let's consider landing this without the unit test and revisiting whether it's worth adding extra complexity.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 14, 2017

Member

Either of two approaches would be easier to understand to me:

  1. Leave it inlined in index.js. Sure, it needs some testing, but I doubt we'll be changing that logic since it's not really dependent on underlying tools (such as Uglify) and tests basic dead code elimination capabilities. No changes in Uglify or bundlers can break it unless they also break DCE (which is exactly what we want to catch).

  2. If you feel strongly about unit testing it, we could put it in packages/react/cjs, so that it ends up in the final build in that folder. We will also need to add this file to whitelist in package.json. Finally, we should be able to import it from Jest with a path like ../../packages/react/cjs/.... Although this feels hacky it would work and avoid complicating build infra for the sake of testing a single file.

Member

gaearon commented Aug 14, 2017

Either of two approaches would be easier to understand to me:

  1. Leave it inlined in index.js. Sure, it needs some testing, but I doubt we'll be changing that logic since it's not really dependent on underlying tools (such as Uglify) and tests basic dead code elimination capabilities. No changes in Uglify or bundlers can break it unless they also break DCE (which is exactly what we want to catch).

  2. If you feel strongly about unit testing it, we could put it in packages/react/cjs, so that it ends up in the final build in that folder. We will also need to add this file to whitelist in package.json. Finally, we should be able to import it from Jest with a path like ../../packages/react/cjs/.... Although this feels hacky it would work and avoid complicating build infra for the sake of testing a single file.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 14, 2017

Member

Writing the test in the first place helped catch another bug and validate that the current version works, so I think was worthwhile to write it, whether we decide to keep it or not.

Totally agree! I just don't see us changing this code often once we've ironed out the first mistakes. We have similar (well, not quite as sophisticated) code for detecting some other minification mistake in Stack, and we also don't have unit tests for it. So far we've never changed it since it was written.

Member

gaearon commented Aug 14, 2017

Writing the test in the first place helped catch another bug and validate that the current version works, so I think was worthwhile to write it, whether we decide to keep it or not.

Totally agree! I just don't see us changing this code often once we've ironed out the first mistakes. We have similar (well, not quite as sophisticated) code for detecting some other minification mistake in Stack, and we also don't have unit tests for it. So far we've never changed it since it was written.

@flarnie

This comment has been minimized.

Show comment
Hide comment
@flarnie

flarnie Aug 14, 2017

Contributor

Nearly have an update ready but I'm switching to getting #10385 landed and running a sync. Will come right back to this.

Contributor

flarnie commented Aug 14, 2017

Nearly have an update ready but I'm switching to getting #10385 landed and running a sync. Will come right back to this.

Inline 'testMinificationUsedDCE' and remove unit test for now
What:
- Inlines the 'testMinificationUsedDCE' method into
  'packages/react/index.js'
- Removes unit test for 'testMinififcationUsedDCE'
- Puts dependencies back the way that they were; should remove extra
  dependencies that were added for the unit test.

Why:
- It would add complexity to the build process to add another file to
  the 'build/packages/react/cjs' folder, and that is the only way to
  pull this out and test it. So instead we are inlining this.
@flarnie

This comment has been minimized.

Show comment
Hide comment
@flarnie

flarnie Aug 16, 2017

Contributor

Re-did manual local testing -

With the minification and no dead code elimination:
screen shot 2017-08-16 at 7 08 50 am

With no minification:
screen shot 2017-08-16 at 7 27 48 am

Contributor

flarnie commented Aug 16, 2017

Re-did manual local testing -

With the minification and no dead code elimination:
screen shot 2017-08-16 at 7 08 50 am

With no minification:
screen shot 2017-08-16 at 7 27 48 am

Show outdated Hide outdated yarn.lock
@gaearon

Looks good, thanks for fixing bugs!

Accepting with changes:

  • Reverting unintentional changes
  • Moving toString back into try catch

Thanks!

Revert unintentional changes to dependency versions, variable placing
**what is the change?:**
- We had updated and added some dependencies, but ended up reverting
  this in a previous commit. The `yarn.lock` and `package.json` needed
  updated still though.
- There is a call to `Function.toString` which we wanted to wrap in a
  `try/catch` block.

**why make this change?:**
To remove unrelated dependency changes and increase the safety of the
`Function.toString` call.

**test plan:**
Manual testing again

**issue:**
#9589

@flarnie flarnie merged commit 34092a0 into facebook:master Aug 16, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment