loadable-components: isReady returns true even though bundles are not all loaded

🐛 Bug Report

My app will randomly crash depending on the load order of my bundles because loadable()'s isReady doesn’t check that all bundles that are needed are loaded (only the main one is checked).

To Reproduce

Steps to reproduce the behavior:

Ok so this is pretty difficult to reproduce because it depends on the load order of different files

I have the following piece of code:

const LoginComp = loadable(() => import('../Login'));

which is being transpiled by @loadable/babel-plugin & @loadable/webpack-plugin into this:

const LoginComp = Object(_loadable_component__WEBPACK_IMPORTED_MODULE_1__[/* default */ "a"])({
  chunkName: function chunkName() {
    return "Login";
  },
  isReady: function isReady(props) {
    if (true) {
      return !!__webpack_require__.m[this.resolve(props)];
    }

    return false;
  },
  requireAsync: function requireAsync() {
    return Promise.all(/* import() | Login */[__webpack_require__.e(0), __webpack_require__.e(2), __webpack_require__.e(1), __webpack_require__.e(3), __webpack_require__.e(63)]).then(__webpack_require__.bind(null, 250));
  },
  requireSync: function requireSync(props) {
    var id = this.resolve(props);

    if (true) {
      return __webpack_require__(id);
    }

    return eval('module.require')(id);
  },
  resolve: function resolve() {
    if (true) {
      return /*require.resolve*/(250);
    }

    return eval('require.resolve')("../Login");
  }
})

In this piece of code, you can see that requireAsync first loads the bundles 0, 2, 1, 3 and 63 before loading module 250 (module 250 is contained by bundle 63 and depends on code present in 0, 2, 1, 3).

Now, I have a separate piece of code that will eventually cause the whole react tree to re-render as soon as another file is loaded (a workaround for react-router which still uses the old context api - I re-render the react tree once the proper locale file is loaded).

Because the whole react tree re-renders, the component generated by loadable() is re-built from scratch.

When that component constructs, it calls isReady to check if it should use requireSync or requireAsync. But isReady only checks that bundle 63 is loaded because it only checks for module 250. So if 0, 2, 1, or 3 are not loaded yet, it will load module 250 which will crash with cannot read property .call of undefined because module 250 will attempt to load a module from one of the other bundles

Expected behavior

isReady is smart enough to detect whether all modules are loaded or not

Link to repl or repo (highly encouraged)

I will attempt to produce one but it’s very difficult to get the timing right

Run npx envinfo --system --binaries --npmPackages @loadable/component,@loadable/server,@loadable/webpack-plugin,@loadable/babel-plugin --markdown --clipboard

Paste the results here:

## System:
 - OS: macOS 10.14.4
 - CPU: (4) x64 Intel(R) Core(TM) i7-6567U CPU @ 3.30GHz
 - Memory: 54.16 MB / 16.00 GB
 - Shell: 3.2.57 - /bin/bash
## Binaries:
 - Node: 10.16.1 - /usr/local/bin/node
 - Yarn: 1.2.1 - ~/.yarn/bin/yarn
 - npm: 6.9.0 - /usr/local/bin/npm
 - Watchman: 4.7.0 - /usr/local/bin/watchman
## npmPackages:
 - @loadable/component: ^5.10.1 => 5.10.2 

About this issue

  • Original URL
  • State: closed
  • Created 5 years ago
  • Reactions: 1
  • Comments: 20 (3 by maintainers)

Commits related to this issue

Most upvoted comments

Hi @neoziro, is there a plan for a release with this fix? The bug is pretty painful and it’d be great to be able to move past it. Thanks!

Thank you @theKashey 😃

Waiting for @neoziro approval.

~Attack plan:~

  • ~remove loadSync. We could not use it. Try to read result back from a provided loadable definition.~
  • ~remove isReady as readed from module cache. Just store result (of a promise) in a provided loadable definition~
  • ~so we are going to use loadable definition as a mutable state to store data, and unbound logic from webpack internal realization, we could not rely on.~

~Where loadable definition is the result of babel plugin transformation.~

After talking to @neoziro I’ve realized that my approach would not play well with loadable SSR model. The only way is to change hot resolve is working

resolve: function resolve() {
    if (true) {
      return /*require.resolve*/(250);
    }

    return eval('require.resolve')("../Login");
  }

// to
resolve: function resolve() {
    if (true) {
// aka [__webpack_require__.e(0), __webpack_require__.e(2), __webpack_require__.e(1), __webpack_require__.e(3), __webpack_require__.e(63)]).then(__webpack_require__.bind(null, 250) 
      return [0, 2, 1, 3, 63, 250]
    }

    return eval('require.resolve')("../Login");
  }

How it’s actually might work

const chunks = [];
// overload import
const original = __webpack_require__.e;
__webpack_require__.e = chunkId => chunks.push(e);

// run import function
requireAsync();

// unmock
__webpack_require__.e = original;

// all required chunks are stored in `chunks`

Then isReady, would be aware of all required chunks, and requireSync might require these chunks in the same order (although that not needed).

I don’t quite like this solution, but another way is to resolve all required chunks inside loadable webpack plugin, and I am not sure how…

Ok, there is a very simple way to handle this case:

  • the server is sending chunks, which all be loaded before the application can hydrate
  • if the required chunk is listed among these ones - you are able to use requireSync as well as isReady
  • on SSR, when you can require something synchronously - you are always isReady
  • in all other cases, ie with client-side driven loading - always use requiredAsync, as long as we could not guarantee that chunk is loaded in full.

Plus - on a ServerSide we shall execute requireAsync for the every requireSync call, and if results do not match each other - throw an error (except the cases with the ssr: false flag)

Track module states by our own.

  • on server side all modules could be loaded in sync. It’s safe.
  • on client side all modules could be loaded in async way, as long as we could “wait” for them. It’s safe.
  • on client side we might remember what we did with some loadables, and that would also help with #420

If someone has a better solution than the one implemented, please feel free to submit a PR. Else we have to speak with webpack to add the missing method.