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

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

Commits on Aug 11, 2017

  1. 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 facebook#9589
    flarnie committed Aug 11, 2017
    Configuration menu
    Copy the full SHA
    8e280c8 View commit details
    Browse the repository at this point in the history

Commits on Aug 12, 2017

  1. 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:**
    facebook#9589
    flarnie committed Aug 12, 2017
    Configuration menu
    Copy the full SHA
    441d661 View commit details
    Browse the repository at this point in the history

Commits on Aug 13, 2017

  1. 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:**
    facebook#9589
    flarnie committed Aug 13, 2017
    Configuration menu
    Copy the full SHA
    05be49a View commit details
    Browse the repository at this point in the history
  2. 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:**
    facebook#9589
    flarnie committed Aug 13, 2017
    Configuration menu
    Copy the full SHA
    8fab885 View commit details
    Browse the repository at this point in the history

Commits on Aug 14, 2017

  1. 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:**
    facebook#9589
    flarnie committed Aug 14, 2017
    Configuration menu
    Copy the full SHA
    9a73704 View commit details
    Browse the repository at this point in the history
  2. Run prettier

    flarnie committed Aug 14, 2017
    Configuration menu
    Copy the full SHA
    5204310 View commit details
    Browse the repository at this point in the history
  3. var -> const/let

    flarnie committed Aug 14, 2017
    Configuration menu
    Copy the full SHA
    26ce0ac View commit details
    Browse the repository at this point in the history
  4. 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:**
    facebook#9589
    flarnie committed Aug 14, 2017
    Configuration menu
    Copy the full SHA
    498f8f5 View commit details
    Browse the repository at this point in the history
  5. 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:**
    flarnie committed Aug 14, 2017
    Configuration menu
    Copy the full SHA
    ab6c1f0 View commit details
    Browse the repository at this point in the history

Commits on Aug 16, 2017

  1. 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 committed Aug 16, 2017
    Configuration menu
    Copy the full SHA
    8a1c36b View commit details
    Browse the repository at this point in the history
  2. 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:**
    facebook#9589
    flarnie committed Aug 16, 2017
    Configuration menu
    Copy the full SHA
    0eecddc View commit details
    Browse the repository at this point in the history