From 921d08dc8b90ee295ed2a049c1fc9c4c4976dea9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 14 Apr 2023 21:09:09 -0400 Subject: [PATCH] [Flight] Fallback to importing the whole module instead of encoding every name (#26624) We currently don't just "require" a module by its module id/path. We encode the pair of module id/path AND its export name. That's because with module splitting, a single original module can end up in two or more separate modules by name. Therefore the manifest files need to encode how to require the whole module as well as how to require each export name. In practice, we don't currently use this because we end up forcing Webpack to deopt and keep it together as a single module, and we don't even have the code in the Webpack plugin to write separate values for each export name. The problem is with CJS we don't statically know what all the export names will be. Since these cases will never be module split, we don't really need to know. This changes the Flight requires to first look for the specific name we're loading and then if that name doesn't exist in the manifest we fallback to looking for the `"*"` name containing the entire module and look for the name in there at runtime. We could probably optimize this a bit if we assume that CJS modules on the server never get built with a name. That way we don't have to do the failed lookup. Additionally, since we've recently merged filepath + name into a single string instead of two values, we now have to split those back out by parsing the string. This is especially unfortunate for server references since those should really not reveal what they are but be a hash or something. The solution might just be to split them back out into two separate fields again. cc @shuding --- .../src/ReactFlightClientConfigNodeBundler.js | 25 +++++- .../ReactFlightClientConfigWebpackBundler.js | 65 +++++++++++++--- .../ReactFlightServerConfigWebpackBundler.js | 38 ++++++--- .../src/ReactFlightWebpackPlugin.js | 16 +++- .../src/__tests__/ReactFlightDOM-test.js | 39 ++++++++++ .../__tests__/ReactFlightDOMBrowser-test.js | 77 ++++++++++++++++++- .../src/__tests__/utils/WebpackMock.js | 55 +++++++------ 7 files changed, 263 insertions(+), 52 deletions(-) diff --git a/packages/react-server-dom-webpack/src/ReactFlightClientConfigNodeBundler.js b/packages/react-server-dom-webpack/src/ReactFlightClientConfigNodeBundler.js index b3bd02c054119..7fc449fd68d52 100644 --- a/packages/react-server-dom-webpack/src/ReactFlightClientConfigNodeBundler.js +++ b/packages/react-server-dom-webpack/src/ReactFlightClientConfigNodeBundler.js @@ -39,8 +39,29 @@ export function resolveClientReference( bundlerConfig: SSRManifest, metadata: ClientReferenceMetadata, ): ClientReference { - const resolvedModuleData = bundlerConfig[metadata.id][metadata.name]; - return resolvedModuleData; + const moduleExports = bundlerConfig[metadata.id]; + let resolvedModuleData = moduleExports[metadata.name]; + let name; + if (resolvedModuleData) { + // The potentially aliased name. + name = resolvedModuleData.name; + } else { + // If we don't have this specific name, we might have the full module. + resolvedModuleData = moduleExports['*']; + if (!resolvedModuleData) { + throw new Error( + 'Could not find the module "' + + metadata.id + + '" in the React SSR Manifest. ' + + 'This is probably a bug in the React Server Components bundler.', + ); + } + name = metadata.name; + } + return { + specifier: resolvedModuleData.specifier, + name: name, + }; } export function resolveServerReference( diff --git a/packages/react-server-dom-webpack/src/ReactFlightClientConfigWebpackBundler.js b/packages/react-server-dom-webpack/src/ReactFlightClientConfigWebpackBundler.js index 85aaab79a9056..1c739c88b1c50 100644 --- a/packages/react-server-dom-webpack/src/ReactFlightClientConfigWebpackBundler.js +++ b/packages/react-server-dom-webpack/src/ReactFlightClientConfigWebpackBundler.js @@ -40,17 +40,31 @@ export function resolveClientReference( metadata: ClientReferenceMetadata, ): ClientReference { if (bundlerConfig) { - const resolvedModuleData = bundlerConfig[metadata.id][metadata.name]; - if (metadata.async) { - return { - id: resolvedModuleData.id, - chunks: resolvedModuleData.chunks, - name: resolvedModuleData.name, - async: true, - }; + const moduleExports = bundlerConfig[metadata.id]; + let resolvedModuleData = moduleExports[metadata.name]; + let name; + if (resolvedModuleData) { + // The potentially aliased name. + name = resolvedModuleData.name; } else { - return resolvedModuleData; + // If we don't have this specific name, we might have the full module. + resolvedModuleData = moduleExports['*']; + if (!resolvedModuleData) { + throw new Error( + 'Could not find the module "' + + metadata.id + + '" in the React SSR Manifest. ' + + 'This is probably a bug in the React Server Components bundler.', + ); + } + name = metadata.name; } + return { + id: resolvedModuleData.id, + chunks: resolvedModuleData.chunks, + name: name, + async: !!metadata.async, + }; } return metadata; } @@ -59,8 +73,37 @@ export function resolveServerReference( bundlerConfig: ServerManifest, id: ServerReferenceId, ): ClientReference { - // This needs to return async: true if it's an async module. - return bundlerConfig[id]; + let name = ''; + let resolvedModuleData = bundlerConfig[id]; + if (resolvedModuleData) { + // The potentially aliased name. + name = resolvedModuleData.name; + } else { + // We didn't find this specific export name but we might have the * export + // which contains this name as well. + // TODO: It's unfortunate that we now have to parse this string. We should + // probably go back to encoding path and name separately on the client reference. + const idx = id.lastIndexOf('#'); + if (idx !== -1) { + name = id.substr(idx + 1); + resolvedModuleData = bundlerConfig[id.substr(0, idx)]; + } + if (!resolvedModuleData) { + throw new Error( + 'Could not find the module "' + + id + + '" in the React Server Manifest. ' + + 'This is probably a bug in the React Server Components bundler.', + ); + } + } + // TODO: This needs to return async: true if it's an async module. + return { + id: resolvedModuleData.id, + chunks: resolvedModuleData.chunks, + name: name, + async: false, + }; } // The chunk cache contains all the chunks we've preloaded so far. diff --git a/packages/react-server-dom-webpack/src/ReactFlightServerConfigWebpackBundler.js b/packages/react-server-dom-webpack/src/ReactFlightServerConfigWebpackBundler.js index 444f2c0b2a150..49c1b2c1b7947 100644 --- a/packages/react-server-dom-webpack/src/ReactFlightServerConfigWebpackBundler.js +++ b/packages/react-server-dom-webpack/src/ReactFlightServerConfigWebpackBundler.js @@ -58,17 +58,37 @@ export function resolveClientReferenceMetadata( config: ClientManifest, clientReference: ClientReference, ): ClientReferenceMetadata { - const resolvedModuleData = config[clientReference.$$id]; - if (clientReference.$$async) { - return { - id: resolvedModuleData.id, - chunks: resolvedModuleData.chunks, - name: resolvedModuleData.name, - async: true, - }; + const modulePath = clientReference.$$id; + let name = ''; + let resolvedModuleData = config[modulePath]; + if (resolvedModuleData) { + // The potentially aliased name. + name = resolvedModuleData.name; } else { - return resolvedModuleData; + // We didn't find this specific export name but we might have the * export + // which contains this name as well. + // TODO: It's unfortunate that we now have to parse this string. We should + // probably go back to encoding path and name separately on the client reference. + const idx = modulePath.lastIndexOf('#'); + if (idx !== -1) { + name = modulePath.substr(idx + 1); + resolvedModuleData = config[modulePath.substr(0, idx)]; + } + if (!resolvedModuleData) { + throw new Error( + 'Could not find the module "' + + modulePath + + '" in the React Client Manifest. ' + + 'This is probably a bug in the React Server Components bundler.', + ); + } } + return { + id: resolvedModuleData.id, + chunks: resolvedModuleData.chunks, + name: name, + async: !!clientReference.$$async, + }; } export function getServerReferenceId( diff --git a/packages/react-server-dom-webpack/src/ReactFlightWebpackPlugin.js b/packages/react-server-dom-webpack/src/ReactFlightWebpackPlugin.js index 298bd4d9b7250..096f5ce0d1dc4 100644 --- a/packages/react-server-dom-webpack/src/ReactFlightWebpackPlugin.js +++ b/packages/react-server-dom-webpack/src/ReactFlightWebpackPlugin.js @@ -247,10 +247,6 @@ export default class ReactFlightWebpackPlugin { return; } - const moduleProvidedExports = compilation.moduleGraph - .getExportsInfo(module) - .getProvidedExports(); - const href = pathToFileURL(module.resource).href; if (href !== undefined) { @@ -267,6 +263,13 @@ export default class ReactFlightWebpackPlugin { specifier: href, name: '*', }; + + // TODO: If this module ends up split into multiple modules, then + // we should encode each the chunks needed for the specific export. + // When the module isn't split, it doesn't matter and we can just + // encode the id of the whole module. This code doesn't currently + // deal with module splitting so is likely broken from ESM anyway. + /* clientManifest[href + '#'] = { id, chunks: chunkIds, @@ -277,6 +280,10 @@ export default class ReactFlightWebpackPlugin { name: '', }; + const moduleProvidedExports = compilation.moduleGraph + .getExportsInfo(module) + .getProvidedExports(); + if (Array.isArray(moduleProvidedExports)) { moduleProvidedExports.forEach(function (name) { clientManifest[href + '#' + name] = { @@ -290,6 +297,7 @@ export default class ReactFlightWebpackPlugin { }; }); } + */ ssrManifest[id] = ssrExports; } diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js index b2a25dec436da..cb30243215fc5 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js @@ -334,6 +334,45 @@ describe('ReactFlightDOM', () => { expect(container.innerHTML).toBe('

Hello World

'); }); + // @gate enableUseHook + it('should be able to render a module split named component export', async () => { + const Module = { + // This gets split into a separate module from the original one. + split: function ({greeting}) { + return greeting + ' World'; + }, + }; + + function Print({response}) { + return

{use(response)}

; + } + + function App({response}) { + return ( + Loading...}> + + + ); + } + + const {split: Component} = clientExports(Module); + + const {writable, readable} = getTestStream(); + const {pipe} = ReactServerDOMServer.renderToPipeableStream( + , + webpackMap, + ); + pipe(writable); + const response = ReactServerDOMClient.createFromReadableStream(readable); + + const container = document.createElement('div'); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(); + }); + expect(container.innerHTML).toBe('

Hello World

'); + }); + // @gate enableUseHook it('should unwrap async module references', async () => { const AsyncModule = Promise.resolve(function AsyncModule({text}) { diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js index d53a9df2e3eb8..0321c5f75d18d 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js @@ -75,12 +75,35 @@ describe('ReactFlightDOMBrowser', () => { } function requireServerRef(ref) { - const metaData = webpackServerMap[ref]; - const mod = __webpack_require__(metaData.id); - if (metaData.name === '*') { + let name = ''; + let resolvedModuleData = webpackServerMap[ref]; + if (resolvedModuleData) { + // The potentially aliased name. + name = resolvedModuleData.name; + } else { + // We didn't find this specific export name but we might have the * export + // which contains this name as well. + // TODO: It's unfortunate that we now have to parse this string. We should + // probably go back to encoding path and name separately on the client reference. + const idx = ref.lastIndexOf('#'); + if (idx !== -1) { + name = ref.substr(idx + 1); + resolvedModuleData = webpackServerMap[ref.substr(0, idx)]; + } + if (!resolvedModuleData) { + throw new Error( + 'Could not find the module "' + + ref + + '" in the React Client Manifest. ' + + 'This is probably a bug in the React Server Components bundler.', + ); + } + } + const mod = __webpack_require__(resolvedModuleData.id); + if (name === '*') { return mod; } - return mod[metaData.name]; + return mod[name]; } async function callServer(actionId, body) { @@ -824,6 +847,52 @@ describe('ReactFlightDOMBrowser', () => { expect(result).toBe('Hello HI'); }); + it('can call a module split server function', async () => { + let actionProxy; + + function Client({action}) { + actionProxy = action; + return 'Click Me'; + } + + function greet(text) { + return 'Hello ' + text; + } + + const ServerModule = serverExports({ + // This gets split into another module + split: greet, + }); + const ClientRef = clientExports(Client); + + const stream = ReactServerDOMServer.renderToReadableStream( + , + webpackMap, + ); + + const response = ReactServerDOMClient.createFromReadableStream(stream, { + async callServer(ref, args) { + const body = await ReactServerDOMClient.encodeReply(args); + return callServer(ref, body); + }, + }); + + function App() { + return use(response); + } + + const container = document.createElement('div'); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(); + }); + expect(container.innerHTML).toBe('Click Me'); + expect(typeof actionProxy).toBe('function'); + + const result = await actionProxy('Split'); + expect(result).toBe('Hello Split'); + }); + it('can bind arguments to a server reference', async () => { let actionProxy; diff --git a/packages/react-server-dom-webpack/src/__tests__/utils/WebpackMock.js b/packages/react-server-dom-webpack/src/__tests__/utils/WebpackMock.js index a911c37602f5b..71b82dbb192eb 100644 --- a/packages/react-server-dom-webpack/src/__tests__/utils/WebpackMock.js +++ b/packages/react-server-dom-webpack/src/__tests__/utils/WebpackMock.js @@ -52,11 +52,6 @@ exports.clientModuleError = function clientModuleError(moduleError) { chunks: [], name: '*', }; - webpackClientMap[path + '#'] = { - id: idx, - chunks: [], - name: '', - }; const mod = {exports: {}}; nodeCompile.call(mod, '"use client"', idx); return mod.exports; @@ -71,11 +66,14 @@ exports.clientExports = function clientExports(moduleExports) { chunks: [], name: '*', }; - webpackClientMap[path + '#'] = { - id: idx, - chunks: [], - name: '', - }; + // We only add this if this test is testing ESM compat. + if ('__esModule' in moduleExports) { + webpackClientMap[path + '#'] = { + id: idx, + chunks: [], + name: '', + }; + } if (typeof moduleExports.then === 'function') { moduleExports.then( asyncModuleExports => { @@ -90,11 +88,16 @@ exports.clientExports = function clientExports(moduleExports) { () => {}, ); } - for (const name in moduleExports) { - webpackClientMap[path + '#' + name] = { - id: idx, + if ('split' in moduleExports) { + // If we're testing module splitting, we encode this name in a separate module id. + const splitIdx = '' + webpackModuleIdx++; + webpackClientModules[splitIdx] = { + s: moduleExports.split, + }; + webpackClientMap[path + '#split'] = { + id: splitIdx, chunks: [], - name: name, + name: 's', }; } const mod = {exports: {}}; @@ -112,16 +115,24 @@ exports.serverExports = function serverExports(moduleExports) { chunks: [], name: '*', }; - webpackServerMap[path + '#'] = { - id: idx, - chunks: [], - name: '', - }; - for (const name in moduleExports) { - webpackServerMap[path + '#' + name] = { + // We only add this if this test is testing ESM compat. + if ('__esModule' in moduleExports) { + webpackServerMap[path + '#'] = { id: idx, chunks: [], - name: name, + name: '', + }; + } + if ('split' in moduleExports) { + // If we're testing module splitting, we encode this name in a separate module id. + const splitIdx = '' + webpackModuleIdx++; + webpackServerModules[splitIdx] = { + s: moduleExports.split, + }; + webpackServerMap[path + '#split'] = { + id: splitIdx, + chunks: [], + name: 's', }; } const mod = {exports: moduleExports};