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

runtime~main.xxxx.js is inserted as inline script in generated index.html #5309

Closed
stereobooster opened this issue Oct 5, 2018 · 16 comments
Closed
Milestone

Comments

@stereobooster
Copy link
Contributor

stereobooster commented Oct 5, 2018

Is this a bug report?

Not sure. It seems like bug, but maybe there is a reason for this behaviour.

Did you try recovering your dependencies?

Yes

Which terms did you search for in User Guide?

None

Environment

  System:
    OS: macOS High Sierra 10.13.3
    CPU: x64 Intel(R) Core(TM) i5-5257U CPU @ 2.70GHz
  Binaries:
    Node: 9.10.1 - ~/.nvm/versions/node/v9.10.1/bin/node
    Yarn: 1.5.1 - /usr/local/bin/yarn
    npm: 5.6.0 - ~/.nvm/versions/node/v9.10.1/bin/npm
  Browsers:
    Chrome: 69.0.3497.100
    Firefox: 62.0.2
    Safari: 11.0.3
  npmPackages:
    react: ^16.4.2 => 16.5.0 
    react-dom: ^16.4.2 => 16.5.0 
    react-scripts: 2.0.4 => 2.0.3 
  npmGlobalPackages:
    create-react-app: Not Found

Steps to Reproduce

  1. yarn build
  2. inspect build/index.html
  3. find <script type="text/javascript">... right below <div id="root" />, which contains the same JS as in build/static/js/runtime~main.xxxx.js

Expected Behavior

I expect to see <script src="static/js/runtime~main.xxxx.js"> instead of inline script

Actual Behavior

As I said it inlines runtime~main.xxxx.js for some reason.

Reproducible Demo

npx create-react-app cra2bug
cd cra2bug
yarn build

react-scripts v2.0.4

@gaearon
Copy link
Contributor

gaearon commented Oct 5, 2018

That’s intentional. We want to inline this chunk because it’s usually small and isn’t worth a separate request. But some people use asset manifest from CRA with a custom HTML so we want to make it easy for them to read it.

@gaearon gaearon closed this as completed Oct 5, 2018
@stereobooster
Copy link
Contributor Author

stereobooster commented Oct 5, 2018

We want to inline this chunk because it’s usually small and isn’t worth a separate request

For the record it is about 3Kb in my application (not sure if it is a lot or not).

Update: also it can break in case of strict CSP

@gaearon
Copy link
Contributor

gaearon commented Oct 5, 2018

For the record it is about 3Kb in my application (not sure if it is a lot or not).

It would still be a waste to send a separate HTTP request for a 3Kb file, unless you reload the page on every navigation (in which case you can add client-side routing?)

Update: also it can break in case of strict CSP

See #5288

@whawker
Copy link

whawker commented Oct 8, 2018

Seems crazy to me to force the inline of a single script to save 1 request, and breaking the biggest win of CSP.

Taken from Google's guide to CSP

Banning inline script is the biggest security win CSP provides

As I see it, users of CRA implementing CSP will run their app, and likely see a console error similar to this
screenshot 2018-10-08 at 11 49 01

This likely directs users down the path of least resistance (i.e the wrong path) by adding script-src: "unsafe-inline" to get rid of the error, and opening up a potential XSS security hole in their application?

@0xdeafcafe
Copy link
Contributor

0xdeafcafe commented Oct 8, 2018

Yeah, this really doesn't seem to make sense. I don't want to have to add a weird build step where it tries to inject a nonce into the built index.html using some fucky sed trick, but I really don't want to add "unsafe-inline".

It would still be a waste to send a separate HTTP request for a 3Kb file, unless you reload the page on every navigation (in which case you can add client-side routing?)

Isn't that was caching/HTTP2 is for?

@stereobooster
Copy link
Contributor Author

I hope it won't turn into battle as it was with ServiceWorkers. (This wasn't my intention when I opened issue)

Also we can lock this ticket and continue conversation after #5288, which should address "unsafe-inline" issue.

@gaearon
Copy link
Contributor

gaearon commented Oct 8, 2018

Isn't that was caching/HTTP2 is for?

HTTP2 is very far from being widely adopted. Caching doesn’t help because the runtime chunk will be different on every build. That’s the exact reason why it exists — to separate it from less often changing code.

I’m fine with adding an environment variable that disables the inlining. Wanna send a PR?

@0xdeafcafe
Copy link
Contributor

That's great. I've never contributed to this project before, so I'll have a try at adding that now. Thanks!

@Timer
Copy link
Contributor

Timer commented Oct 8, 2018

Probably look for INLINE_SCRIPT=false?

@peterbe
Copy link

peterbe commented Oct 8, 2018

@gaearon

HTTP2 is very far from being widely adopted.

https://caniuse.com/#search=http2 says only IE 11, Opera Mini and "UC Browser for Android" (whatever that is) does NOT support HTTP2.

So for the majority of browsers, inline scripting small scripts makes the performance worse, ...in theory. I haven't done extensive measurements to support this but if the HTML document (the build/index.html) can be delivered slightly sooner, the browser can spawn other important tasks sooner such as downloading linked style sheets, scripts and etc.

Also, leaving it on by default does encourage the risk of people letting their CSP policy include 'unsafe-inline'.

Having said all of that, I appreciate that I have a very limited context on the run-up to this change in 2.0 and perhaps I've missed some other subtle analysis of the advantages of inlining.

What I'm suggesting is that it does not inline unless you set INLINE_SMALL_SCRIPTS=true yarn run build.

@peterbe
Copy link

peterbe commented Oct 8, 2018

Probably look for INLINE_SCRIPT=false?

I'm guessing this was just a rough suggestion but let's at least denote that the inlining is based on some webpack rule that deems the script very small.

@peterbe
Copy link

peterbe commented Oct 8, 2018

Lastly about the web performance; inlining is often better than a blocking resource. I.e. it's better to serve a 700KB (than a 600KB) HTML document if it means you don't have to block the parsing until all .js files have been downloaded and parsed. With HTTP2, the blocking hurts a LOT less than it used to.

Ideally, what you should do is <script async src="/static/...> the script tags. Then they don't require a new HTTP connection to a different SSL/DNS and it won't block rendering of what little HTML you get on the first GET. Most build/index.html files are just <body><div id="root"></div><script...></body> so it's not a huge deal anyway.

@gaearon
Copy link
Contributor

gaearon commented Oct 8, 2018

If you have a specific proposal for how it should work (along with how to make webpack emit and work with <script async>), you're welcome to file a separate proposal for discussion, or a PR. I agree our current solution is not ideal but the fact that a better solution is possible in theory shouldn’t stop us from making incremental improvements.

https://caniuse.com/#search=http2 says only IE 11, Opera Mini and "UC Browser for Android" (whatever that is) does NOT support HTTP2.

I was talking about what people use in production today, not about what they could possibly use based on browser support. Adoption depends on hosting, server side solutions and many other factors. Industry-wide, HTTP2 adoption is still low.

@Timer Timer added this to the 2.0.5 milestone Oct 8, 2018
@gaearon gaearon modified the milestones: 2.0.5, 2.0.6 Oct 14, 2018
@gaearon
Copy link
Contributor

gaearon commented Oct 14, 2018

Out in 2.0.5

@gaearon gaearon closed this as completed Oct 14, 2018
@jamespizzurro
Copy link

jamespizzurro commented Oct 18, 2018

Unless I'm mistaken, this doesn't appear to be resolved in 2.0.5; an inline script is still being added to build/index.html such that I'm seeing the following error when trying to use the React app I create using create-react-app in a Chrome extension:

Refused to execute inline script because it violates the following Content Security Policy directive: "script-src 'self' blob: filesystem: chrome-extension-resource:". Either the 'unsafe-inline' keyword, a hash ('sha256-GgRxrVOKNdB4LrRsVPDSbzvfdV4UqglmviH9GoBJ5jk='), or a nonce ('nonce-...') is required to enable inline execution.

Am I mistaken in assuming this should not be happening anymore under 2.0.5?

EDIT: It looks like there was some discussion in this issue and others about adding an environment variable. Is that what @gaearon was referring to with regards to it being "out in 2.0.5," or was that referring to something else? Apologies for the confusion.

EDIT 2: Sorry, just saw #5354 and INLINE_RUNTIME_CHUNK in the README here. Thanks!

@Timer Timer modified the milestones: 2.0.6, 2.1 Oct 22, 2018
@lock lock bot locked and limited conversation to collaborators Jan 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants