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

Issue 400 #445

Merged
merged 3 commits into from
Dec 2, 2019
Merged

Issue 400 #445

merged 3 commits into from
Dec 2, 2019

Conversation

theKashey
Copy link
Collaborator

@theKashey theKashey commented Oct 8, 2019

Summary

Track the loading state for isReady, should solve #400

The logic so:

  • there is no __webpack_modules__[key] - load async
  • there is __webpack_modules__[key] - load sync, only if you are still not loading async

In other words - don't load while you load.

Test plan

Hard to reproduce, you have to load component, which would require more than one chunk to load, and the first one to be loaded before second.

@foglerek
Copy link

@neoziro Do you think this approach is going to end up getting merged in, or are you planning on waiting on webpack to expose a more reliable sync-load method?

mikaelgramont added a commit to surfingdirt/web that referenced this pull request Oct 24, 2019
@theKashey
Copy link
Collaborator Author

👍

@theKashey
Copy link
Collaborator Author

loading was not the best name, as long as in case of failed loading it would be still loading, while it does nothing.
As a result, it was replaced by resolved

@iamstarkov
Copy link

@theKashey should the tests be fixed?

@theKashey
Copy link
Collaborator Author

Sorry, forgot to push updated snapshots

@theKashey
Copy link
Collaborator Author

@neoziro - only you can merge and publish

@mschipperheyn
Copy link

@neoziro @theKashey any chance this can get merged soonish? The main disadvantage for monorepos is that you can't install directly by git commit.

@ilyaagarkov
Copy link

Hi everyone!
@neoziro @theKashey Any updates?

@gregberge gregberge merged commit 3024348 into master Dec 2, 2019
@gregberge gregberge deleted the issue-400 branch December 6, 2019 14:10
toolness added a commit to JustFixNYC/tenants2 that referenced this pull request Apr 27, 2021
This upgrades our `@loadable` packages to their latest versions.

This started as an attempt at trying to figure out what upgrade broke the tests in #2055 (a full upgrade to webpack 5 and all webpack plugins, including `@loadable` packages).  Specifically, the ultimate effect of the breakage was that all our static pages returned "Loading locale data..." instead of their actual content, which suggested that the loadable locale bundles weren't being loaded on the server-side when rendering static markup.

I debugged the problem by individually upgrading each `@loadable` package until the breakage manifested itself.  I narrowed it down to something in between `@loadable/babel-plugin` 5.10.3 and 5.11.0.  It looked like this was due to gregberge/loadable-components#445.

After looking at `@loadable`'s [`createLoadable.js`](https://github.com/gregberge/loadable-components/blob/b640c82a742ffbccc423439e2e205d1becdf5491/packages/component/src/createLoadable.js), I noticed that it was detecting whether it was running on the server-side (and therefore whether to import synchronously vs. asynchronously) based on whether it was being rendered in the context of a `ChunkExtractorManager`.  I then noticed that our static markup rendering logic wasn't actually wrapping the rendering in one of those.  Once I changed this, the tests passed.

Additionally, this upgrades our `browserslist` package, because it happened to be convenient.
fivethreeo pushed a commit to fivethreeo/loadable-components that referenced this pull request Nov 16, 2022
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

Successfully merging this pull request may close these issues.

6 participants