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.

Sign up for GitHub

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

Jump to bottom

Non-dead JavaScript is removed during build #7448

Open
salomvary opened this issue Jul 29, 2019 · 5 comments
Open

Non-dead JavaScript is removed during build #7448

salomvary opened this issue Jul 29, 2019 · 5 comments
Labels
issue: bug

Comments

@salomvary
Copy link

salomvary commented Jul 29, 2019 •
edited
Loading

Describe the bug

react-scripts build seems to be removing inline JavaScript code from index.html that should not be removed. It happens to code paths that are conditional and the condition contains a build-time '%VARIABLE%'.

Workaround: move the build-time variable outside of the condition.

Did you try recovering your dependencies?

No, it's a newly created project.

Which terms did you search for in User Guide?

NODE_ENV, index.html, variables

Environment

  System:
    OS: macOS 10.14.5
    CPU: (4) x64 Intel(R) Core(TM) i5-5287U CPU @ 2.90GHz
  Binaries:
    Node: 8.16.0 - /usr/local/bin/node
    Yarn: 1.17.3 - /usr/local/bin/yarn
    npm: 6.10.1-salomvary1 - /usr/local/bin/npm
  Browsers:
    Chrome: 75.0.3770.142
    Firefox: 68.0.1
    Safari: 12.1.1
  npmPackages:
    react: ^16.8.6 => 16.8.6 
    react-dom: ^16.8.6 => 16.8.6 
    react-scripts: 3.0.1 => 3.0.1 
  npmGlobalPackages:
    create-react-app: 3.0.1

Steps to reproduce

  1. Create a new app with create-react-app.
  2. Edit public/index.html and add a the snipped below after <body>.
  3. Run npm run build.
  4. Open build/index.html
    <script>
      console.log('%NODE_ENV%');
      var nodeEnv = '%NODE_ENV%';
      if ('%NODE_ENV%' === 'production') {
        console.log('This is production!');
      }
      if (nodeEnv === 'production') {
        console.log('This is REALLY production!');
      }
    </script>

Expected behavior

build/index.html should contain the minified version of the entire snippet with %NODE_ENV% replaced with production.

Actual behavior

This is what build/index.html contains instead (note that the first if block is entirely missing):

<script type="text/javascript">
console.log("production");var nodeEnv="production";"production"===nodeEnv&&console.log("This is REALLY production!")
</script>

Reproducible demo

https://github.com/salomvary/create-react-app-bug
@heyimalex
Copy link
Contributor

I think this is being handled in #6449

@miraage
Copy link

miraage commented Jul 30, 2019

You run REACT_APP_STAGE=... but refer as NODE_ENV which is not correct.

@salomvary
Copy link
Author

You run REACT_APP_STAGE=... but refer as NODE_ENV which is not correct.

@miraage Well, it's technically correct but irrelevant for the reproduction example. Updated the description and removed it to avoid further confusion.

@miraage
Copy link

miraage commented Jul 30, 2019 •
edited
Loading

@salomvary I have completely missed the point you're raising. This is very interesting...
Webpack DefinePlugin replaces occurence with a quoted value (e.g. if you write process.env.NODE_ENV === 'production', you get "production" === 'production').
One may expect InterpolatePlugin behaving in the same matter, but apparently not. Both code and docs (docs are not mentioning that InterpolatePlugin behaves differently compare to DefinePlugin) need to be updated.

@heyimalex
Copy link
Contributor

The PR I linked to handles it. The issue is in the order the plugins run: minification is happening before interpolation, so '%NODE_ENV%' === 'production' is removed before %NODE_ENV% is replaced with production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
issue: bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@salomvary @miraage @heyimalex