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

[Flight] Instrument the Promise for Async Module instead of using a Module Cache #26985

Merged
merged 4 commits into from
Jun 28, 2023

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Jun 21, 2023

Currently, since we use a module cache for async modules, it doesn't automatically get updated when the module registry gets updated (HMR).

This technique ensures that if Webpack replaces the module (HMR) then we'll get the new Promise when we require it again.

This technique doesn't work for ESM and probably not Vite since ESM will provide a new Promise each time you call import() but in the Webpack/CJS approach this Promise is an entry in the module cache and not a promise for the entry.

I tried to replicate the original issue in the fixture but it's tricky to replicate because 1) we can't really use async modules the same way without compiling both server and client 2) even then I'm not quite sure how to repro the HMR issue.

This is a bit tricky to test because it really requires both the server
and the client to be bundled so that the server knows it would've been
an async import so it can tag it as such.
We typically do this for Promises in general. Usually the ones we provide.

This ensures that if Webpack replaces the module (HMR) then we'll get the
new Promise when we require it again.
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jun 21, 2023
@react-sizebot
Copy link

react-sizebot commented Jun 21, 2023

Comparing: d1c8cda...705df6d

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 164.56 kB 164.56 kB = 51.83 kB 51.83 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 171.97 kB 171.97 kB = 54.06 kB 54.07 kB
facebook-www/ReactDOM-prod.classic.js = 571.47 kB 571.47 kB = 100.78 kB 100.78 kB
facebook-www/ReactDOM-prod.modern.js = 555.25 kB 555.25 kB = 97.96 kB 97.96 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.production.min.js +0.81% 10.02 kB 10.10 kB +0.84% 4.04 kB 4.08 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.production.min.js +0.81% 10.02 kB 10.10 kB +0.84% 4.04 kB 4.08 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.production.min.js +0.81% 10.02 kB 10.10 kB +0.84% 4.04 kB 4.08 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.development.js +0.76% 50.69 kB 51.08 kB +0.79% 12.59 kB 12.69 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.development.js +0.76% 50.69 kB 51.08 kB +0.79% 12.59 kB 12.69 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.development.js +0.76% 50.69 kB 51.08 kB +0.79% 12.59 kB 12.69 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.unbundled.development.js +0.38% 94.73 kB 95.09 kB +0.41% 22.64 kB 22.74 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.unbundled.development.js +0.38% 94.73 kB 95.09 kB +0.41% 22.64 kB 22.74 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.unbundled.development.js +0.38% 94.73 kB 95.09 kB +0.41% 22.64 kB 22.74 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.unbundled.production.min.js +0.29% 23.17 kB 23.23 kB +0.15% 8.17 kB 8.18 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.unbundled.production.min.js +0.29% 23.17 kB 23.23 kB +0.15% 8.17 kB 8.18 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.unbundled.production.min.js +0.29% 23.17 kB 23.23 kB +0.15% 8.17 kB 8.18 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.production.min.js +0.24% 10.40 kB 10.43 kB = 4.23 kB 4.21 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.production.min.js +0.24% 10.40 kB 10.43 kB = 4.23 kB 4.21 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.production.min.js +0.24% 10.40 kB 10.43 kB = 4.23 kB 4.21 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.production.min.js +0.23% 10.66 kB 10.69 kB = 4.30 kB 4.29 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.production.min.js +0.23% 10.66 kB 10.69 kB = 4.30 kB 4.29 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.production.min.js +0.23% 10.66 kB 10.69 kB = 4.30 kB 4.29 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.production.min.js +0.23% 10.68 kB 10.71 kB = 4.28 kB 4.26 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.production.min.js +0.23% 10.68 kB 10.71 kB = 4.28 kB 4.26 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.production.min.js +0.23% 10.68 kB 10.71 kB = 4.28 kB 4.26 kB
oss-experimental/react-server-dom-webpack/umd/react-server-dom-webpack-client.browser.production.min.js +0.22% 10.94 kB 10.97 kB = 4.37 kB 4.37 kB
oss-stable-semver/react-server-dom-webpack/umd/react-server-dom-webpack-client.browser.production.min.js +0.22% 10.94 kB 10.97 kB = 4.37 kB 4.37 kB
oss-stable/react-server-dom-webpack/umd/react-server-dom-webpack-client.browser.production.min.js +0.22% 10.94 kB 10.97 kB = 4.37 kB 4.37 kB

Generated by 🚫 dangerJS against 705df6d

@@ -111,7 +111,33 @@ export function resolveServerReference<T>(
// in Webpack but unfortunately it's not exposed so we have to
// replicate it in user space. null means that it has already loaded.
const chunkCache: Map<string, null | Promise<any>> = new Map();
const asyncModuleCache: Map<string, Thenable<any>> = new Map();

function requireAsyncModule(id: string): null | Thenable<any> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't love the name. My intuition is this has some parity with the requireModule function below but it's really about preloading only. maybe loadAsyncModule?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then it sounds it should be using the load API. It doesn't load the module itself, it requires it which might load something that isn't even a module or nothing at all.

The confusing part here is that there's two naming conventions requireAsyncModule is kind of a webpack/cjs thing, where as requireModule is more of a Flight concept so it names it. We should probably rename requireModule to something else because "require" is kind of CJS specific name for that concept.

@sebmarkbage sebmarkbage merged commit 5945e06 into facebook:main Jun 28, 2023
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…odule Cache (facebook#26985)

Currently, since we use a module cache for async modules, it doesn't
automatically get updated when the module registry gets updated (HMR).

This technique ensures that if Webpack replaces the module (HMR) then
we'll get the new Promise when we require it again.

This technique doesn't work for ESM and probably not Vite since ESM will
provide a new Promise each time you call `import()` but in the
Webpack/CJS approach this Promise is an entry in the module cache and
not a promise for the entry.

I tried to replicate the original issue in the fixture but it's tricky
to replicate because 1) we can't really use async modules the same way
without compiling both server and client 2) even then I'm not quite sure
how to repro the HMR issue.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
…odule Cache (#26985)

Currently, since we use a module cache for async modules, it doesn't
automatically get updated when the module registry gets updated (HMR).

This technique ensures that if Webpack replaces the module (HMR) then
we'll get the new Promise when we require it again.

This technique doesn't work for ESM and probably not Vite since ESM will
provide a new Promise each time you call `import()` but in the
Webpack/CJS approach this Promise is an entry in the module cache and
not a promise for the entry.

I tried to replicate the original issue in the fixture but it's tricky
to replicate because 1) we can't really use async modules the same way
without compiling both server and client 2) even then I'm not quite sure
how to repro the HMR issue.

DiffTrain build for commit 5945e06.
unstubbable added a commit to vercel/next.js that referenced this pull request Aug 12, 2024
…8535)

The first iteration of `deleteAppClientCache()` was introduced in #41510. Its purpose was to remove stale client components (for SSR) from React's cache. Since React didn't (and still doesn't) offer an API for that, this was accomplished by deleting `react-server-dom-webpack` from the require cache.

With the bundling that was introduced in #55362, `deleteAppClientCache()` was changed to delete the whole server runtimes (e.g. `app-page.runtime.dev.js`) from the require cache. This has the unfortunate side effect that also React's other module scope cache (back then `ReactCurrentCache`, now `ReactSharedInternals.A`) is reset between compilations. As a result, when fetching data from a route handler in a page, the fetch cache didn't work properly. This issue was masked though by response buffering for the static generation cache. In #68447 the response buffering will be removed which [uncovered the issue](https://github.com/vercel/next.js/actions/runs/10217197012/job/28270497496).

React had two module-scoped caches for client components:
- `chunkCache` – caches the loaded JS chunks, as specified in the SSR manifest
- `asyncModuleCache` – caches the required modules, if marked as `async` in the SSR manifest

The `asyncModuleCache` was subsequently dropped in facebook/react#26985.

In addition, with Webpack, we don't create any client component chunks for SSR (removed from the manifest in #50959). So there's no need anymore to delete server runtime bundles from the require cache, and we can remove `deleteAppClientCache()` from the `NextJsRequireCacheHotReloader`.

For Turbopack, the exported `deleteAppClientCache` function is still used because Turbopack does create SSR client component chunks. Ideally, React would offer an API to clear the chunk cache. But for now we can keep this logic, since Turbopack also handles this a bit more sophisticated than the Webpack plugin, so that the aforementioned fetch issue does not occur there.

This PR in conjunction with #68193 should then also fix the issue that was reported in #52126.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants