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

Error when combining with react-loadable. #66

Open
judewang opened this issue Sep 23, 2017 · 18 comments
Open

Error when combining with react-loadable. #66

judewang opened this issue Sep 23, 2017 · 18 comments

Comments

@judewang
Copy link

Hi, I followed guide in create-react-app and done code splitting with react-loadable. After deploying my app to server, I found an error in console said Can't find variable: webpackJsonp . I then found the problem is that react-snap will overwrite the html but inject chunks inside <head>, the chunk's content is something like webpackJsonp(...). Since the webpackJsonp was defined inside main.js and main.js is at the bottom of <body>, that cause the problem.

Here's the sample code produced by npm run build && npm run react-snap

<!DOCTYPE html>
<html lang="en">

<head>
  <meta charset="utf-8">
  <meta name="viewport"
        content="width=device-width,initial-scale=1,shrink-to-fit=no">
  <meta name="theme-color"
        content="#000000">
  <link rel="manifest"
        href="/manifest.json">
  <link rel="shortcut icon"
        href="/favicon.ico">
  <title>React App</title>
  <link href="/static/css/main.cacbacc7.css"
        rel="stylesheet">
  <style type="text/css"
         data-styled-components="kCrotB hzkseL"
         data-styled-components-is-local="true">
    /* sc-component-id: sc-bdVaJa */

    .sc-bdVaJa {}

    .kCrotB {
      display: -webkit-box;
      display: -webkit-flex;
      display: -ms-flexbox;
      display: flex;
      height: 100vh;
      width: 100vw;
    }
    /* sc-component-id: sc-bwzfXH */

    .sc-bwzfXH {}

    .hzkseL {
      font-size: 48px;
      text-align: center;
      margin: auto;
    }

  </style>
  <script type="text/javascript"
          charset="utf-8"
          src="/static/js/0.7749783b.chunk.js"></script>
</head>

<body><noscript>You need to enable JavaScript to run this app.</noscript>
  <div id="root">
    <div class="App"
         data-reactroot=""
         data-reactid="1"
         data-react-checksum="1111271051">
      <div class="App-header"
           data-reactid="2">
        <h2 data-reactid="3">Welcome to React</h2></div>
      <ul data-reactid="4">
        <li data-reactid="5"><a class="active"
             style="text-decoration:none;"
             aria-current="true"
             href="/"
             data-reactid="6">Home</a></li>
        <li data-reactid="7"><a aria-current="false"
             href="/about"
             data-reactid="8">About</a></li>
      </ul>
      <div class="sc-bdVaJa kCrotB"
           data-reactid="9">
        <p class="sc-bwzfXH hzkseL"
           data-reactid="10">Loading...</p>
      </div>
    </div>
  </div>
  <script type="text/javascript"
          src="/static/js/main.9652c51a.js"></script>
</body>

</html>
@stereobooster
Copy link

Can you post small example of application. This will help a lot with debugging

@judewang
Copy link
Author

@judewang judewang changed the title Error when combining react-loadable. Error when combining with react-loadable. Sep 25, 2017
@jasan-s
Copy link

jasan-s commented Oct 23, 2017

@judewang did you ever find a solution?

@judewang
Copy link
Author

@jasan-s not yet. I stop finding any solutions but try using server side rendering.

@jasan-s
Copy link

jasan-s commented Oct 23, 2017

What are you using for SSR?

@judewang
Copy link
Author

@jasan-s Just ReactDOMServer.renderToString

@stereobooster
Copy link

The actual problem here is the thing which is responsible for injecting JS (react-loadable I suppose), it should inject all JS after the last one in document instead of injecting them in head.

@stereobooster
Copy link

stereobooster commented Oct 23, 2017

I suppose this one https://github.com/webpack/webpack/blob/b8f181f57fdc33fc8fccd99e07a3684c9fb723d6/lib/JsonpMainTemplatePlugin.js#L105

"var head = document.getElementsByTagName('head')[0];",
this.applyPluginsWaterfall("jsonp-script", "", chunk, hash),
"head.appendChild(script);",

Instead it should be something like this:

var lastScript = document.scripts[document.scripts.length - 1];
lastScript.parentNode.insertBefore(newNode, lastScript);

This needs correspondent ticket in webpack repo

@stereobooster
Copy link

I have "fix" for this https://github.com/judewang/snapshot-issue/pull/1/files

@stereobooster
Copy link

stereobooster commented Oct 24, 2017

The question is: does this thing makes sense. The idea behind loadable and async import is to speed up loading of website, but after pre-renderer it will contain all js files referenced in html - so it will download them one by one in blocking manner and evaluate.

Other options

  • use async, so browser will download files without blocking
  • use <link rel="preload" as="script"> so browser will download files, but not evaluate them
  • remove chunks from the document, so main bundle will be responsible for downloading it

Not sure if first or second option is faster, but third is the slowest.

@aheissenberger
Copy link

why do you think option 3 will be the slowest? Isn't this the only solution where only the script needed for this page are loaded?

@stereobooster
Copy link

why do you think option 3 will be the slowest?

well, we just rendered page - we know what scripts are gonna be rendered anyway. So if we will not preload them, they will be loaded in waterfall manner, this is definitely slower

The only tricky case here if your page has different components depending on the screen size, then you need to prerender page for mobile phones.

@geelen
Copy link
Owner

geelen commented Nov 2, 2017

@stereobooster so does every chunk get added to the HTML in react-snap? Or only the chunks for the current render.

So if we will not preload them, they will be loaded in waterfall manner, this is definitely slower

They'll be loaded simultaneously, but executed sequentially, which is correct. Using async means they could be executed out-of-order, if I'm not mistaken.

The only tricky case here if your page has different components depending on the screen size, then you need to prerender page for mobile phones.

This is starting to wade into dangerous/exciting territory. I've opened #83 to continue the discussion

@stereobooster
Copy link

so does every chunk get added to the HTML in react-snap? Or only the chunks for the current render.

only the chunks for the current render.

They'll be loaded simultaneously, but executed sequentially, which is correct.

I mean if we will not use any preload technique, chunks will be downloaded by webpack mechanism, which will kick in only after main will be downloaded and parsed and starts to execute.

But I forgot about the case when chunks depend on each other, that is why async potentially a problem, so <link rel=preload> is the best solution here. It is not supported by IE, but should not break anything either

@geelen
Copy link
Owner

geelen commented Nov 2, 2017

Ohh right of course, Webpack is never going to expect that the data it's asked to lazy-load is already in the DOM, so preload just means that that data can be already on its way when Webpack asks for it. Right?

@stereobooster
Copy link

on its way when Webpack asks for it. Right?

Yes.

@judewang judewang closed this as completed Nov 9, 2017
@judewang judewang reopened this Nov 9, 2017
@gijo-varghese
Copy link

Is there any fix for this?

@judewang
Copy link
Author

judewang commented Sep 29, 2018

@gijo-varghese There's a "fix" in the comment above, but you'd better read #66 (comment), too.

In our team, now we use Next.js for React app which needs SSR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants