From 565148d7516f9f0c4d90a7f06ab479c79ad56c69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 30 Nov 2020 17:25:56 -0500 Subject: [PATCH 1/6] Disallow *.server.js imports from any other files (#20309) This convention ensures that you can declare that you intend for a file to only be used on the server (even if it technically might resolve on the client). --- .../flight/server/{index.js => cli.server.js} | 0 fixtures/flight/server/package.json | 3 +- .../src/ReactFlightWebpackNodeLoader.js | 29 ++++++++++++++++++- .../src/ReactFlightWebpackNodeRegister.js | 25 ++++++++++++++++ scripts/rollup/bundles.js | 2 +- 5 files changed, 56 insertions(+), 3 deletions(-) rename fixtures/flight/server/{index.js => cli.server.js} (100%) diff --git a/fixtures/flight/server/index.js b/fixtures/flight/server/cli.server.js similarity index 100% rename from fixtures/flight/server/index.js rename to fixtures/flight/server/cli.server.js diff --git a/fixtures/flight/server/package.json b/fixtures/flight/server/package.json index 5bbefffbabe..59055eee7d5 100644 --- a/fixtures/flight/server/package.json +++ b/fixtures/flight/server/package.json @@ -1,3 +1,4 @@ { - "type": "commonjs" + "type": "commonjs", + "main": "./cli.server.js" } diff --git a/packages/react-transport-dom-webpack/src/ReactFlightWebpackNodeLoader.js b/packages/react-transport-dom-webpack/src/ReactFlightWebpackNodeLoader.js index f9fd2a54e24..102da330287 100644 --- a/packages/react-transport-dom-webpack/src/ReactFlightWebpackNodeLoader.js +++ b/packages/react-transport-dom-webpack/src/ReactFlightWebpackNodeLoader.js @@ -31,12 +31,39 @@ type GetSourceFunction = ( type Source = string | ArrayBuffer | Uint8Array; +let warnedAboutConditionsFlag = false; + export async function resolve( specifier: string, context: ResolveContext, defaultResolve: ResolveFunction, ): Promise { - // TODO: Resolve server-only files. + if (!context.conditions.includes('react-server')) { + context = { + ...context, + conditions: [...context.conditions, 'react-server'], + }; + if (!warnedAboutConditionsFlag) { + warnedAboutConditionsFlag = true; + // eslint-disable-next-line react-internal/no-production-logging + console.warn( + 'You did not run Node.js with the `--conditions react-server` flag. ' + + 'Any "react-server" override will only work with ESM imports.', + ); + } + } + // We intentionally check the specifier here instead of the resolved file. + // This allows package exports to configure non-server aliases that resolve to server files + // depending on environment. It's probably a bad idea to export a server file as "main" though. + if (specifier.endsWith('.server.js')) { + if (context.parentURL && !context.parentURL.endsWith('.server.js')) { + throw new Error( + `Cannot import "${specifier}" from "${context.parentURL}". ` + + 'By react-server convention, .server.js files can only be imported from other .server.js files. ' + + 'That way nobody accidentally sends these to the client by indirectly importing it.', + ); + } + } return defaultResolve(specifier, context, defaultResolve); } diff --git a/packages/react-transport-dom-webpack/src/ReactFlightWebpackNodeRegister.js b/packages/react-transport-dom-webpack/src/ReactFlightWebpackNodeRegister.js index f5149be1c42..17c3b8ef9d9 100644 --- a/packages/react-transport-dom-webpack/src/ReactFlightWebpackNodeRegister.js +++ b/packages/react-transport-dom-webpack/src/ReactFlightWebpackNodeRegister.js @@ -9,6 +9,9 @@ const url = require('url'); +// $FlowFixMe +const Module = require('module'); + module.exports = function register() { (require: any).extensions['.client.js'] = function(module, path) { module.exports = { @@ -16,4 +19,26 @@ module.exports = function register() { name: url.pathToFileURL(path).href, }; }; + + const originalResolveFilename = Module._resolveFilename; + + Module._resolveFilename = function(request, parent, isMain, options) { + // We intentionally check the request here instead of the resolved file. + // This allows package exports to configure non-server aliases that resolve to server files + // depending on environment. It's probably a bad idea to export a server file as "main" though. + if (request.endsWith('.server.js')) { + if ( + parent && + parent.filename && + !parent.filename.endsWith('.server.js') + ) { + throw new Error( + `Cannot import "${request}" from "${parent.filename}". ` + + 'By react-server convention, .server.js files can only be imported from other .server.js files. ' + + 'That way nobody accidentally sends these to the client by indirectly importing it.', + ); + } + } + return originalResolveFilename.apply(this, arguments); + }; }; diff --git a/scripts/rollup/bundles.js b/scripts/rollup/bundles.js index f39d197eb03..641e6dd2602 100644 --- a/scripts/rollup/bundles.js +++ b/scripts/rollup/bundles.js @@ -310,7 +310,7 @@ const bundles = [ moduleType: RENDERER_UTILS, entry: 'react-transport-dom-webpack/node-register', global: 'ReactFlightWebpackNodeRegister', - externals: ['url'], + externals: ['url', 'module'], }, /******* React Transport DOM Server Relay *******/ From 3f73dcee374732a331b7e72c88ff6dd3b7116a42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 30 Nov 2020 17:37:27 -0500 Subject: [PATCH 2/6] Support named exports from client references (#20312) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Rename "name"->"filepath" field on Webpack module references This field name will get confused with the imported name or the module id. * Switch back to transformSource instead of getSource getSource would be more efficient in the cases where we don't need to read the original file but we'll need to most of the time. Even then, we can't return a JS file if we're trying to support non-JS loader because it'll end up being transformed. Similarly, we'll need to parse the file and we can't parse it before it's transformed. So we need to chain with other loaders that know how. * Add acorn dependency This should be the version used by Webpack since we have a dependency on Webpack anyway. * Parse exported names of ESM modules We need to statically resolve the names that a client component will export so that we can export a module reference for each of the names. For export * from, this gets tricky because we need to also load the source of the next file to parse that. We don't know exactly how the client is built so we guess it's somewhat default. * Handle imported names one level deep in CommonJS using a Proxy We use a proxy to see what property the server access and that will tell us which property we'll want to import on the client. * Add export name to module reference and Webpack map To support named exports each name needs to be encoded as a separate reference. It's possible with module splitting that different exports end up in different chunks. It's also possible that the export is renamed as part of minification. So the map also includes a map from the original to the bundled name. * Special case plain CJS requires and conditional imports using __esModule This models if the server tries to import .default or a plain require. We should replicate the same thing on the client when we load that module reference. * Dedupe acorn-related deps Co-authored-by: Mateusz Burzyński --- fixtures/flight/loader/index.js | 14 +- fixtures/flight/server/handler.server.js | 33 ++- fixtures/flight/src/App.server.js | 4 +- fixtures/flight/src/Counter.client.js | 2 +- fixtures/flight/src/Counter2.client.js | 1 + .../react-transport-dom-webpack/package.json | 1 + .../ReactFlightClientWebpackBundlerConfig.js | 13 +- .../ReactFlightServerWebpackBundlerConfig.js | 9 +- .../src/ReactFlightWebpackNodeLoader.js | 200 +++++++++++++++++- .../src/ReactFlightWebpackNodeRegister.js | 53 ++++- .../src/__tests__/ReactFlightDOM-test.js | 10 +- scripts/flow/environment.js | 2 +- scripts/rollup/bundles.js | 2 +- yarn.lock | 39 ++-- 14 files changed, 322 insertions(+), 61 deletions(-) create mode 100644 fixtures/flight/src/Counter2.client.js diff --git a/fixtures/flight/loader/index.js b/fixtures/flight/loader/index.js index b9cfa5b73ee..04960e6dcc9 100644 --- a/fixtures/flight/loader/index.js +++ b/fixtures/flight/loader/index.js @@ -1,4 +1,8 @@ -import {resolve, getSource} from 'react-transport-dom-webpack/node-loader'; +import { + resolve, + getSource, + transformSource as reactTransformSource, +} from 'react-transport-dom-webpack/node-loader'; export {resolve, getSource}; @@ -13,7 +17,7 @@ const babelOptions = { ], }; -export async function transformSource(source, context, defaultTransformSource) { +async function babelTransformSource(source, context, defaultTransformSource) { const {format} = context; if (format === 'module') { const opt = Object.assign({filename: context.url}, babelOptions); @@ -22,3 +26,9 @@ export async function transformSource(source, context, defaultTransformSource) { } return defaultTransformSource(source, context, defaultTransformSource); } + +export async function transformSource(source, context, defaultTransformSource) { + return reactTransformSource(source, context, (s, c) => { + return babelTransformSource(s, c, defaultTransformSource); + }); +} diff --git a/fixtures/flight/server/handler.server.js b/fixtures/flight/server/handler.server.js index d7715af9cc7..86476bb2c2d 100644 --- a/fixtures/flight/server/handler.server.js +++ b/fixtures/flight/server/handler.server.js @@ -17,14 +17,35 @@ module.exports = async function(req, res) { pipeToNodeWritable(, res, { // TODO: Read from a map on the disk. [resolve('../src/Counter.client.js')]: { - id: './src/Counter.client.js', - chunks: ['1'], - name: 'default', + Counter: { + id: './src/Counter.client.js', + chunks: ['2'], + name: 'Counter', + }, + }, + [resolve('../src/Counter2.client.js')]: { + Counter: { + id: './src/Counter2.client.js', + chunks: ['1'], + name: 'Counter', + }, }, [resolve('../src/ShowMore.client.js')]: { - id: './src/ShowMore.client.js', - chunks: ['2'], - name: 'default', + default: { + id: './src/ShowMore.client.js', + chunks: ['3'], + name: 'default', + }, + '': { + id: './src/ShowMore.client.js', + chunks: ['3'], + name: '', + }, + '*': { + id: './src/ShowMore.client.js', + chunks: ['3'], + name: '*', + }, }, }); }; diff --git a/fixtures/flight/src/App.server.js b/fixtures/flight/src/App.server.js index 54a644dc48d..35a223dce8b 100644 --- a/fixtures/flight/src/App.server.js +++ b/fixtures/flight/src/App.server.js @@ -2,7 +2,8 @@ import * as React from 'react'; import Container from './Container.js'; -import Counter from './Counter.client.js'; +import {Counter} from './Counter.client.js'; +import {Counter as Counter2} from './Counter2.client.js'; import ShowMore from './ShowMore.client.js'; @@ -11,6 +12,7 @@ export default function App() {

Hello, world

+

Lorem ipsum

 diff --git a/fixtures/flight/src/Counter.client.js b/fixtures/flight/src/Counter.client.js index 00a1f2cbe44..676280f0542 100644 --- a/fixtures/flight/src/Counter.client.js +++ b/fixtures/flight/src/Counter.client.js @@ -2,7 +2,7 @@ import * as React from 'react'; import Container from './Container.js'; -export default function Counter() { +export function Counter() { const [count, setCount] = React.useState(0); return ( diff --git a/fixtures/flight/src/Counter2.client.js b/fixtures/flight/src/Counter2.client.js new file mode 100644 index 00000000000..084f7bc5f07 --- /dev/null +++ b/fixtures/flight/src/Counter2.client.js @@ -0,0 +1 @@ +export * from './Counter.client.js'; diff --git a/packages/react-transport-dom-webpack/package.json b/packages/react-transport-dom-webpack/package.json index a71a558f516..8e2db02610e 100644 --- a/packages/react-transport-dom-webpack/package.json +++ b/packages/react-transport-dom-webpack/package.json @@ -50,6 +50,7 @@ "webpack": "^4.43.0" }, "dependencies": { + "acorn": "^6.2.1", "loose-envify": "^1.1.0", "object-assign": "^4.1.1" }, diff --git a/packages/react-transport-dom-webpack/src/ReactFlightClientWebpackBundlerConfig.js b/packages/react-transport-dom-webpack/src/ReactFlightClientWebpackBundlerConfig.js index 3e667960d26..f3c4e1bf1c1 100644 --- a/packages/react-transport-dom-webpack/src/ReactFlightClientWebpackBundlerConfig.js +++ b/packages/react-transport-dom-webpack/src/ReactFlightClientWebpackBundlerConfig.js @@ -59,5 +59,16 @@ export function requireModule(moduleData: ModuleReference): T { throw entry; } } - return __webpack_require__(moduleData.id)[moduleData.name]; + const moduleExports = __webpack_require__(moduleData.id); + if (moduleData.name === '*') { + // This is a placeholder value that represents that the caller imported this + // as a CommonJS module as is. + return moduleExports; + } + if (moduleData.name === '') { + // This is a placeholder value that represents that the caller accessed the + // default property of this if it was an ESM interop module. + return moduleExports.__esModule ? moduleExports.default : moduleExports; + } + return moduleExports[moduleData.name]; } diff --git a/packages/react-transport-dom-webpack/src/ReactFlightServerWebpackBundlerConfig.js b/packages/react-transport-dom-webpack/src/ReactFlightServerWebpackBundlerConfig.js index f691809522c..c8469eeba80 100644 --- a/packages/react-transport-dom-webpack/src/ReactFlightServerWebpackBundlerConfig.js +++ b/packages/react-transport-dom-webpack/src/ReactFlightServerWebpackBundlerConfig.js @@ -8,7 +8,9 @@ */ type WebpackMap = { - [filename: string]: ModuleMetaData, + [filepath: string]: { + [name: string]: ModuleMetaData, + }, }; export type BundlerConfig = WebpackMap; @@ -16,6 +18,7 @@ export type BundlerConfig = WebpackMap; // eslint-disable-next-line no-unused-vars export type ModuleReference = { $$typeof: Symbol, + filepath: string, name: string, }; @@ -30,7 +33,7 @@ export type ModuleKey = string; const MODULE_TAG = Symbol.for('react.module.reference'); export function getModuleKey(reference: ModuleReference): ModuleKey { - return reference.name; + return reference.filepath + '#' + reference.name; } export function isModuleReference(reference: Object): boolean { @@ -41,5 +44,5 @@ export function resolveModuleMetaData( config: BundlerConfig, moduleReference: ModuleReference, ): ModuleMetaData { - return config[moduleReference.name]; + return config[moduleReference.filepath][moduleReference.name]; } diff --git a/packages/react-transport-dom-webpack/src/ReactFlightWebpackNodeLoader.js b/packages/react-transport-dom-webpack/src/ReactFlightWebpackNodeLoader.js index 102da330287..d716cda4653 100644 --- a/packages/react-transport-dom-webpack/src/ReactFlightWebpackNodeLoader.js +++ b/packages/react-transport-dom-webpack/src/ReactFlightWebpackNodeLoader.js @@ -7,6 +7,8 @@ * @flow */ +import acorn from 'acorn'; + type ResolveContext = { conditions: Array, parentURL: string | void, @@ -16,11 +18,10 @@ type ResolveFunction = ( string, ResolveContext, ResolveFunction, -) => Promise; +) => Promise<{url: string}>; type GetSourceContext = { format: string, - url: string, }; type GetSourceFunction = ( @@ -29,15 +30,32 @@ type GetSourceFunction = ( GetSourceFunction, ) => Promise<{source: Source}>; +type TransformSourceContext = { + format: string, + url: string, +}; + +type TransformSourceFunction = ( + Source, + TransformSourceContext, + TransformSourceFunction, +) => Promise<{source: Source}>; + type Source = string | ArrayBuffer | Uint8Array; let warnedAboutConditionsFlag = false; +let stashedGetSource: null | GetSourceFunction = null; +let stashedResolve: null | ResolveFunction = null; + export async function resolve( specifier: string, context: ResolveContext, defaultResolve: ResolveFunction, -): Promise { +): Promise<{url: string}> { + // We stash this in case we end up needing to resolve export * statements later. + stashedResolve = defaultResolve; + if (!context.conditions.includes('react-server')) { context = { ...context, @@ -71,14 +89,174 @@ export async function getSource( url: string, context: GetSourceContext, defaultGetSource: GetSourceFunction, +) { + // We stash this in case we end up needing to resolve export * statements later. + stashedGetSource = defaultGetSource; + return defaultGetSource(url, context, defaultGetSource); +} + +function addExportNames(names, node) { + switch (node.type) { + case 'Identifier': + names.push(node.name); + return; + case 'ObjectPattern': + for (let i = 0; i < node.properties.length; i++) + addExportNames(names, node.properties[i]); + return; + case 'ArrayPattern': + for (let i = 0; i < node.elements.length; i++) { + const element = node.elements[i]; + if (element) addExportNames(names, element); + } + return; + case 'Property': + addExportNames(names, node.value); + return; + case 'AssignmentPattern': + addExportNames(names, node.left); + return; + case 'RestElement': + addExportNames(names, node.argument); + return; + case 'ParenthesizedExpression': + addExportNames(names, node.expression); + return; + } +} + +function resolveClientImport( + specifier: string, + parentURL: string, +): Promise<{url: string}> { + // Resolve an import specifier as if it was loaded by the client. This doesn't use + // the overrides that this loader does but instead reverts to the default. + // This resolution algorithm will not necessarily have the same configuration + // as the actual client loader. It should mostly work and if it doesn't you can + // always convert to explicit exported names instead. + const conditions = ['node', 'import']; + if (stashedResolve === null) { + throw new Error( + 'Expected resolve to have been called before transformSource', + ); + } + return stashedResolve(specifier, {conditions, parentURL}, stashedResolve); +} + +async function loadClientImport( + url: string, + defaultTransformSource: TransformSourceFunction, ): Promise<{source: Source}> { - if (url.endsWith('.client.js')) { - // TODO: Named exports. - const src = - "export default { $$typeof: Symbol.for('react.module.reference'), name: " + - JSON.stringify(url) + - '}'; - return {source: src}; + if (stashedGetSource === null) { + throw new Error( + 'Expected getSource to have been called before transformSource', + ); } - return defaultGetSource(url, context, defaultGetSource); + // TODO: Validate that this is another module by calling getFormat. + const {source} = await stashedGetSource( + url, + {format: 'module'}, + stashedGetSource, + ); + return defaultTransformSource( + source, + {format: 'module', url}, + defaultTransformSource, + ); +} + +async function parseExportNamesInto( + transformedSource: string, + names: Array, + parentURL: string, + defaultTransformSource, +): Promise { + const {body} = acorn.parse(transformedSource, { + ecmaVersion: '2019', + sourceType: 'module', + }); + for (let i = 0; i < body.length; i++) { + const node = body[i]; + switch (node.type) { + case 'ExportAllDeclaration': + if (node.exported) { + addExportNames(names, node.exported); + continue; + } else { + const {url} = await resolveClientImport(node.source.value, parentURL); + const {source} = await loadClientImport(url, defaultTransformSource); + if (typeof source !== 'string') { + throw new Error('Expected the transformed source to be a string.'); + } + parseExportNamesInto(source, names, url, defaultTransformSource); + continue; + } + case 'ExportDefaultDeclaration': + names.push('default'); + continue; + case 'ExportNamedDeclaration': + if (node.declaration) { + if (node.declaration.type === 'VariableDeclaration') { + const declarations = node.declaration.declarations; + for (let j = 0; j < declarations.length; j++) { + addExportNames(names, declarations[j].id); + } + } else { + addExportNames(names, node.declaration.id); + } + } + if (node.specificers) { + const specificers = node.specificers; + for (let j = 0; j < specificers.length; j++) { + addExportNames(names, specificers[j].exported); + } + } + continue; + } + } +} + +export async function transformSource( + source: Source, + context: TransformSourceContext, + defaultTransformSource: TransformSourceFunction, +): Promise<{source: Source}> { + const transformed = await defaultTransformSource( + source, + context, + defaultTransformSource, + ); + if (context.format === 'module' && context.url.endsWith('.client.js')) { + const transformedSource = transformed.source; + if (typeof transformedSource !== 'string') { + throw new Error('Expected source to have been transformed to a string.'); + } + + const names = []; + await parseExportNamesInto( + transformedSource, + names, + context.url, + defaultTransformSource, + ); + + let newSrc = + "const MODULE_REFERENCE = Symbol.for('react.module.reference');\n"; + for (let i = 0; i < names.length; i++) { + const name = names[i]; + if (name === 'default') { + newSrc += 'export default '; + } else { + newSrc += 'export const ' + name + ' = '; + } + newSrc += '{ $$typeof: MODULE_REFERENCE, filepath: '; + newSrc += JSON.stringify(context.url); + newSrc += ', name: '; + newSrc += JSON.stringify(name); + newSrc += '};\n'; + } + + return {source: newSrc}; + } + return transformed; } diff --git a/packages/react-transport-dom-webpack/src/ReactFlightWebpackNodeRegister.js b/packages/react-transport-dom-webpack/src/ReactFlightWebpackNodeRegister.js index 17c3b8ef9d9..26a92b8323d 100644 --- a/packages/react-transport-dom-webpack/src/ReactFlightWebpackNodeRegister.js +++ b/packages/react-transport-dom-webpack/src/ReactFlightWebpackNodeRegister.js @@ -13,11 +13,58 @@ const url = require('url'); const Module = require('module'); module.exports = function register() { + const MODULE_REFERENCE = Symbol.for('react.module.reference'); + const proxyHandlers = { + get: function(target, name, receiver) { + switch (name) { + // These names are read by the Flight runtime if you end up using the exports object. + case '$$typeof': + // These names are a little too common. We should probably have a way to + // have the Flight runtime extract the inner target instead. + return target.$$typeof; + case 'filepath': + return target.filepath; + case 'name': + return target.name; + // We need to special case this because createElement reads it if we pass this + // reference. + case 'defaultProps': + return undefined; + case '__esModule': + // Something is conditionally checking which export to use. We'll pretend to be + // an ESM compat module but then we'll check again on the client. + target.default = { + $$typeof: MODULE_REFERENCE, + filepath: target.filepath, + // This a placeholder value that tells the client to conditionally use the + // whole object or just the default export. + name: '', + }; + return true; + } + let cachedReference = target[name]; + if (!cachedReference) { + cachedReference = target[name] = { + $$typeof: MODULE_REFERENCE, + filepath: target.filepath, + name: name, + }; + } + return cachedReference; + }, + set: function() { + throw new Error('Cannot assign to a client module from a server module.'); + }, + }; + (require: any).extensions['.client.js'] = function(module, path) { - module.exports = { - $$typeof: Symbol.for('react.module.reference'), - name: url.pathToFileURL(path).href, + const moduleId = url.pathToFileURL(path).href; + const moduleReference: {[string]: any} = { + $$typeof: MODULE_REFERENCE, + filepath: moduleId, + name: '*', // Represents the whole object instead of a particular import. }; + module.exports = new Proxy(moduleReference, proxyHandlers); }; const originalResolveFilename = Module._resolveFilename; diff --git a/packages/react-transport-dom-webpack/src/__tests__/ReactFlightDOM-test.js b/packages/react-transport-dom-webpack/src/__tests__/ReactFlightDOM-test.js index d8568c661c2..e437c7b20bc 100644 --- a/packages/react-transport-dom-webpack/src/__tests__/ReactFlightDOM-test.js +++ b/packages/react-transport-dom-webpack/src/__tests__/ReactFlightDOM-test.js @@ -68,12 +68,14 @@ describe('ReactFlightDOM', () => { d: moduleExport, }; webpackMap['path/' + idx] = { - id: '' + idx, - chunks: [], - name: 'd', + default: { + id: '' + idx, + chunks: [], + name: 'd', + }, }; const MODULE_TAG = Symbol.for('react.module.reference'); - return {$$typeof: MODULE_TAG, name: 'path/' + idx}; + return {$$typeof: MODULE_TAG, filepath: 'path/' + idx, name: 'default'}; } async function waitForSuspense(fn) { diff --git a/scripts/flow/environment.js b/scripts/flow/environment.js index 8fd7111ebf5..3efe41ff344 100644 --- a/scripts/flow/environment.js +++ b/scripts/flow/environment.js @@ -68,4 +68,4 @@ declare module 'EventListener' { } declare function __webpack_chunk_load__(id: string): Promise; -declare function __webpack_require__(id: string): {default: any}; +declare function __webpack_require__(id: string): any; diff --git a/scripts/rollup/bundles.js b/scripts/rollup/bundles.js index 641e6dd2602..6f0f88e04e0 100644 --- a/scripts/rollup/bundles.js +++ b/scripts/rollup/bundles.js @@ -301,7 +301,7 @@ const bundles = [ moduleType: RENDERER_UTILS, entry: 'react-transport-dom-webpack/node-loader', global: 'ReactFlightWebpackNodeLoader', - externals: [], + externals: ['acorn'], }, /******* React Transport DOM Webpack Node.js CommonJS Loader *******/ diff --git a/yarn.lock b/yarn.lock index d44e85f4e47..7b6cb089096 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2617,15 +2617,10 @@ acorn-globals@^6.0.0: acorn "^7.1.1" acorn-walk "^7.1.1" -acorn-jsx@^5.0.0: - version "5.1.0" - resolved "https://registry.yarnpkg.com/acorn-jsx/-/acorn-jsx-5.1.0.tgz#294adb71b57398b0680015f0a38c563ee1db5384" - integrity sha512-tMUqwBWfLFbJbizRmEcWSLw6HnFzfdJs2sOJEOwwtVPMoH/0Ay+E703oZz78VSXZiiDcZrQ5XKjPIUQixhmgVw== - -acorn-jsx@^5.2.0: - version "5.2.0" - resolved "https://registry.yarnpkg.com/acorn-jsx/-/acorn-jsx-5.2.0.tgz#4c66069173d6fdd68ed85239fc256226182b2ebe" - integrity sha512-HiUX/+K2YpkpJ+SzBffkM/AQ2YE03S0U1kjTLVpoJdhZMOWy8qvXVN9JdLqv2QsaQ6MPYQIuNmwD8zOiYUofLQ== +acorn-jsx@^5.0.0, acorn-jsx@^5.2.0: + version "5.3.1" + resolved "https://registry.yarnpkg.com/acorn-jsx/-/acorn-jsx-5.3.1.tgz#fc8661e11b7ac1539c47dbfea2e72b3af34d267b" + integrity sha512-K0Ptm/47OKfQRpNQ2J/oIN/3QYiK6FwW+eJbILhsdxh2WTLdl+30o8aGdTbm5JbffpFFAg/g+zi1E+jvJha5ng== acorn-walk@^6.0.1: version "6.2.0" @@ -2637,25 +2632,15 @@ acorn-walk@^7.1.1: resolved "https://registry.yarnpkg.com/acorn-walk/-/acorn-walk-7.2.0.tgz#0de889a601203909b0fbe07b8938dc21d2e967bc" integrity sha512-OPdCF6GsMIP+Az+aWfAAOEt2/+iVDKE7oy6lJ098aoe59oAmK76qV6Gw60SbZ8jHuG2wH058GF4pLFbYamYrVA== -acorn@^6.0.1, acorn@^6.0.7: - version "6.3.0" - resolved "https://registry.yarnpkg.com/acorn/-/acorn-6.3.0.tgz#0087509119ffa4fc0a0041d1e93a417e68cb856e" - integrity sha512-/czfa8BwS88b9gWQVhc8eknunSA2DoJpJyTQkhheIf5E48u1N0R4q/YxxsAeqRrmK9TQ/uYfgLDfZo91UlANIA== - -acorn@^6.4.1: - version "6.4.1" - resolved "https://registry.yarnpkg.com/acorn/-/acorn-6.4.1.tgz#531e58ba3f51b9dacb9a6646ca4debf5b14ca474" - integrity sha512-ZVA9k326Nwrj3Cj9jlh3wGFutC2ZornPNARZwsNYqQYgN0EsV2d53w5RN/co65Ohn4sUAUtb1rSUAOD6XN9idA== - -acorn@^7.1.0: - version "7.1.0" - resolved "https://registry.yarnpkg.com/acorn/-/acorn-7.1.0.tgz#949d36f2c292535da602283586c2477c57eb2d6c" - integrity sha512-kL5CuoXA/dgxlBbVrflsflzQ3PAas7RYZB52NOm/6839iVYJgKMJ3cQJD+t2i5+qFa8h3MDpEOJiS64E8JLnSQ== +acorn@^6.0.1, acorn@^6.0.7, acorn@^6.2.1, acorn@^6.4.1: + version "6.4.2" + resolved "https://registry.yarnpkg.com/acorn/-/acorn-6.4.2.tgz#35866fd710528e92de10cf06016498e47e39e1e6" + integrity sha512-XtGIhXwF8YM8bJhGxG5kXgjkEuNGLTkoYqVE+KMR+aspr4KGYmKYg7yUe3KghyQ9yheNwLnjmzh/7+gfDBmHCQ== -acorn@^7.1.1, acorn@^7.4.0: - version "7.4.0" - resolved "https://registry.yarnpkg.com/acorn/-/acorn-7.4.0.tgz#e1ad486e6c54501634c6c397c5c121daa383607c" - integrity sha512-+G7P8jJmCHr+S+cLfQxygbWhXy+8YTVGzAkpEbcLo2mLoL7tij/VG41QSHACSf5QgYRhMZYHuNc6drJaO0Da+w== +acorn@^7.1.0, acorn@^7.1.1, acorn@^7.4.0: + version "7.4.1" + resolved "https://registry.yarnpkg.com/acorn/-/acorn-7.4.1.tgz#feaed255973d2e77555b83dbc08851a6c63520fa" + integrity sha512-nQyp0o1/mNdbTO1PO6kHkwSrmgZ0MT/jCCpNiwbUjGoRN4dlBhqJtoQuCnEOKzgTVwg0ZWiCoQy6SxMebQVh8A== adbkit-logcat@^1.1.0: version "1.1.0" From 5711811da17c62de7ba6c911dbc7f20f16bfc980 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 1 Dec 2020 10:14:04 -0500 Subject: [PATCH 3/6] Reconcile element types of lazy component yielding the same type (#20357) * Reconcile element types of lazy component yielding the same type * Add some legacy mode and suspense boundary flushing tests * Fix infinite loop in legacy mode In legacy mode we typically commit the suspending fiber and then rerender the nearest boundary to render the fallback in a separate commit. We can't do that when the boundary itself suspends because when we try to do the second pass, it'll suspend again and infinite loop. Interestingly the legacy semantics are not needed in this case because they exist to let an existing partial render fully commit its partial state. In this case there's no partial state, so we can just render the fallback immediately instead. * Check fast refresh compatibility first resolveLazy can suspend and if it does, it can resuspend. Fast refresh assumes that we don't resuspend. Instead it relies on updating the inner component later. * Use timers instead of act to force fallbacks to show --- .../src/ReactChildFiber.new.js | 114 ++++---- .../src/ReactChildFiber.old.js | 113 ++++---- .../src/ReactFiberThrow.new.js | 10 +- .../src/ReactFiberThrow.old.js | 10 +- .../src/__tests__/ReactLazy-test.internal.js | 262 ++++++++++++++++++ 5 files changed, 407 insertions(+), 102 deletions(-) diff --git a/packages/react-reconciler/src/ReactChildFiber.new.js b/packages/react-reconciler/src/ReactChildFiber.new.js index 601f3b21e43..534d7032ba2 100644 --- a/packages/react-reconciler/src/ReactChildFiber.new.js +++ b/packages/react-reconciler/src/ReactChildFiber.new.js @@ -246,6 +246,12 @@ function warnOnFunctionType(returnFiber: Fiber) { } } +function resolveLazy(lazyType) { + const payload = lazyType._payload; + const init = lazyType._init; + return init(payload); +} + // This wrapper function exists because I expect to clone the code in each path // to be able to optimize each path individually by branching early. This needs // a compiler or we can do it manually. Helpers that don't need this branching @@ -383,11 +389,32 @@ function ChildReconciler(shouldTrackSideEffects) { element: ReactElement, lanes: Lanes, ): Fiber { + const elementType = element.type; + if (elementType === REACT_FRAGMENT_TYPE) { + return updateFragment( + returnFiber, + current, + element.props.children, + lanes, + element.key, + ); + } if (current !== null) { if ( - current.elementType === element.type || + current.elementType === elementType || // Keep this check inline so it only runs on the false path: - (__DEV__ ? isCompatibleFamilyForHotReloading(current, element) : false) + (__DEV__ + ? isCompatibleFamilyForHotReloading(current, element) + : false) || + // Lazy types should reconcile their resolved type. + // We need to do this after the Hot Reloading check above, + // because hot reloading has different semantics than prod because + // it doesn't resuspend. So we can't let the call below suspend. + (enableLazyElements && + typeof elementType === 'object' && + elementType !== null && + elementType.$$typeof === REACT_LAZY_TYPE && + resolveLazy(elementType) === current.type) ) { // Move based on index const existing = useFiber(current, element.props); @@ -551,15 +578,6 @@ function ChildReconciler(shouldTrackSideEffects) { switch (newChild.$$typeof) { case REACT_ELEMENT_TYPE: { if (newChild.key === key) { - if (newChild.type === REACT_FRAGMENT_TYPE) { - return updateFragment( - returnFiber, - oldFiber, - newChild.props.children, - lanes, - key, - ); - } return updateElement(returnFiber, oldFiber, newChild, lanes); } else { return null; @@ -622,15 +640,6 @@ function ChildReconciler(shouldTrackSideEffects) { existingChildren.get( newChild.key === null ? newIdx : newChild.key, ) || null; - if (newChild.type === REACT_FRAGMENT_TYPE) { - return updateFragment( - returnFiber, - matchedFiber, - newChild.props.children, - lanes, - newChild.key, - ); - } return updateElement(returnFiber, matchedFiber, newChild, lanes); } case REACT_PORTAL_TYPE: { @@ -1101,39 +1110,44 @@ function ChildReconciler(shouldTrackSideEffects) { // TODO: If key === null and child.key === null, then this only applies to // the first item in the list. if (child.key === key) { - switch (child.tag) { - case Fragment: { - if (element.type === REACT_FRAGMENT_TYPE) { - deleteRemainingChildren(returnFiber, child.sibling); - const existing = useFiber(child, element.props.children); - existing.return = returnFiber; - if (__DEV__) { - existing._debugSource = element._source; - existing._debugOwner = element._owner; - } - return existing; + const elementType = element.type; + if (elementType === REACT_FRAGMENT_TYPE) { + if (child.tag === Fragment) { + deleteRemainingChildren(returnFiber, child.sibling); + const existing = useFiber(child, element.props.children); + existing.return = returnFiber; + if (__DEV__) { + existing._debugSource = element._source; + existing._debugOwner = element._owner; } - break; + return existing; } - default: { - if ( - child.elementType === element.type || - // Keep this check inline so it only runs on the false path: - (__DEV__ - ? isCompatibleFamilyForHotReloading(child, element) - : false) - ) { - deleteRemainingChildren(returnFiber, child.sibling); - const existing = useFiber(child, element.props); - existing.ref = coerceRef(returnFiber, child, element); - existing.return = returnFiber; - if (__DEV__) { - existing._debugSource = element._source; - existing._debugOwner = element._owner; - } - return existing; + } else { + if ( + child.elementType === elementType || + // Keep this check inline so it only runs on the false path: + (__DEV__ + ? isCompatibleFamilyForHotReloading(child, element) + : false) || + // Lazy types should reconcile their resolved type. + // We need to do this after the Hot Reloading check above, + // because hot reloading has different semantics than prod because + // it doesn't resuspend. So we can't let the call below suspend. + (enableLazyElements && + typeof elementType === 'object' && + elementType !== null && + elementType.$$typeof === REACT_LAZY_TYPE && + resolveLazy(elementType) === child.type) + ) { + deleteRemainingChildren(returnFiber, child.sibling); + const existing = useFiber(child, element.props); + existing.ref = coerceRef(returnFiber, child, element); + existing.return = returnFiber; + if (__DEV__) { + existing._debugSource = element._source; + existing._debugOwner = element._owner; } - break; + return existing; } } // Didn't match. diff --git a/packages/react-reconciler/src/ReactChildFiber.old.js b/packages/react-reconciler/src/ReactChildFiber.old.js index adb9c0418df..df4c2e10d92 100644 --- a/packages/react-reconciler/src/ReactChildFiber.old.js +++ b/packages/react-reconciler/src/ReactChildFiber.old.js @@ -246,6 +246,12 @@ function warnOnFunctionType(returnFiber: Fiber) { } } +function resolveLazy(lazyType) { + const payload = lazyType._payload; + const init = lazyType._init; + return init(payload); +} + // This wrapper function exists because I expect to clone the code in each path // to be able to optimize each path individually by branching early. This needs // a compiler or we can do it manually. Helpers that don't need this branching @@ -383,11 +389,32 @@ function ChildReconciler(shouldTrackSideEffects) { element: ReactElement, lanes: Lanes, ): Fiber { + const elementType = element.type; + if (elementType === REACT_FRAGMENT_TYPE) { + return updateFragment( + returnFiber, + current, + element.props.children, + lanes, + element.key, + ); + } if (current !== null) { if ( - current.elementType === element.type || + current.elementType === elementType || // Keep this check inline so it only runs on the false path: - (__DEV__ ? isCompatibleFamilyForHotReloading(current, element) : false) + (__DEV__ + ? isCompatibleFamilyForHotReloading(current, element) + : false) || + // Lazy types should reconcile their resolved type. + // We need to do this after the Hot Reloading check above, + // because hot reloading has different semantics than prod because + // it doesn't resuspend. So we can't let the call below suspend. + (enableLazyElements && + typeof elementType === 'object' && + elementType !== null && + elementType.$$typeof === REACT_LAZY_TYPE && + resolveLazy(elementType) === current.type) ) { // Move based on index const existing = useFiber(current, element.props); @@ -551,15 +578,6 @@ function ChildReconciler(shouldTrackSideEffects) { switch (newChild.$$typeof) { case REACT_ELEMENT_TYPE: { if (newChild.key === key) { - if (newChild.type === REACT_FRAGMENT_TYPE) { - return updateFragment( - returnFiber, - oldFiber, - newChild.props.children, - lanes, - key, - ); - } return updateElement(returnFiber, oldFiber, newChild, lanes); } else { return null; @@ -622,15 +640,6 @@ function ChildReconciler(shouldTrackSideEffects) { existingChildren.get( newChild.key === null ? newIdx : newChild.key, ) || null; - if (newChild.type === REACT_FRAGMENT_TYPE) { - return updateFragment( - returnFiber, - matchedFiber, - newChild.props.children, - lanes, - newChild.key, - ); - } return updateElement(returnFiber, matchedFiber, newChild, lanes); } case REACT_PORTAL_TYPE: { @@ -1101,39 +1110,43 @@ function ChildReconciler(shouldTrackSideEffects) { // TODO: If key === null and child.key === null, then this only applies to // the first item in the list. if (child.key === key) { - switch (child.tag) { - case Fragment: { - if (element.type === REACT_FRAGMENT_TYPE) { - deleteRemainingChildren(returnFiber, child.sibling); - const existing = useFiber(child, element.props.children); - existing.return = returnFiber; - if (__DEV__) { - existing._debugSource = element._source; - existing._debugOwner = element._owner; - } - return existing; + const elementType = element.type; + if (elementType === REACT_FRAGMENT_TYPE) { + if (child.tag === Fragment) { + deleteRemainingChildren(returnFiber, child.sibling); + const existing = useFiber(child, element.props.children); + existing.return = returnFiber; + if (__DEV__) { + existing._debugSource = element._source; + existing._debugOwner = element._owner; } - break; + return existing; } - default: { - if ( - child.elementType === element.type || - // Keep this check inline so it only runs on the false path: - (__DEV__ - ? isCompatibleFamilyForHotReloading(child, element) - : false) - ) { - deleteRemainingChildren(returnFiber, child.sibling); - const existing = useFiber(child, element.props); - existing.ref = coerceRef(returnFiber, child, element); - existing.return = returnFiber; - if (__DEV__) { - existing._debugSource = element._source; - existing._debugOwner = element._owner; - } - return existing; + } else { + if ( + child.elementType === elementType || + (__DEV__ + ? isCompatibleFamilyForHotReloading(child, element) + : false) || + // Lazy types should reconcile their resolved type. + // We need to do this after the Hot Reloading check above, + // because hot reloading has different semantics than prod because + // it doesn't resuspend. So we can't let the call below suspend. + (enableLazyElements && + typeof elementType === 'object' && + elementType !== null && + elementType.$$typeof === REACT_LAZY_TYPE && + resolveLazy(elementType) === child.type) + ) { + deleteRemainingChildren(returnFiber, child.sibling); + const existing = useFiber(child, element.props); + existing.ref = coerceRef(returnFiber, child, element); + existing.return = returnFiber; + if (__DEV__) { + existing._debugSource = element._source; + existing._debugOwner = element._owner; } - break; + return existing; } } // Didn't match. diff --git a/packages/react-reconciler/src/ReactFiberThrow.new.js b/packages/react-reconciler/src/ReactFiberThrow.new.js index 9dd9df6d0cf..01ab69cecee 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.new.js +++ b/packages/react-reconciler/src/ReactFiberThrow.new.js @@ -256,7 +256,15 @@ function throwException( // Note: It doesn't matter whether the component that suspended was // inside a blocking mode tree. If the Suspense is outside of it, we // should *not* suspend the commit. - if ((workInProgress.mode & BlockingMode) === NoMode) { + // + // If the suspense boundary suspended itself suspended, we don't have to + // do this trick because nothing was partially started. We can just + // directly do a second pass over the fallback in this render and + // pretend we meant to render that directly. + if ( + (workInProgress.mode & BlockingMode) === NoMode && + workInProgress !== returnFiber + ) { workInProgress.flags |= DidCapture; sourceFiber.flags |= ForceUpdateForLegacySuspense; diff --git a/packages/react-reconciler/src/ReactFiberThrow.old.js b/packages/react-reconciler/src/ReactFiberThrow.old.js index 8d338d552d7..fabcffa8737 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.old.js +++ b/packages/react-reconciler/src/ReactFiberThrow.old.js @@ -256,7 +256,15 @@ function throwException( // Note: It doesn't matter whether the component that suspended was // inside a blocking mode tree. If the Suspense is outside of it, we // should *not* suspend the commit. - if ((workInProgress.mode & BlockingMode) === NoMode) { + // + // If the suspense boundary suspended itself suspended, we don't have to + // do this trick because nothing was partially started. We can just + // directly do a second pass over the fallback in this render and + // pretend we meant to render that directly. + if ( + (workInProgress.mode & BlockingMode) === NoMode && + workInProgress !== returnFiber + ) { workInProgress.flags |= DidCapture; sourceFiber.flags |= ForceUpdateForLegacySuspense; diff --git a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js index 55c15e7412e..b7f1570823c 100644 --- a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js @@ -1268,6 +1268,192 @@ describe('ReactLazy', () => { expect(componentStackMessage).toContain('in Lazy'); }); + // @gate enableLazyElements + it('mount and reorder lazy types', async () => { + class Child extends React.Component { + componentDidMount() { + Scheduler.unstable_yieldValue('Did mount: ' + this.props.label); + } + componentDidUpdate() { + Scheduler.unstable_yieldValue('Did update: ' + this.props.label); + } + render() { + return ; + } + } + + function ChildA({lowerCase}) { + return ; + } + + function ChildB({lowerCase}) { + return ; + } + + const LazyChildA = lazy(() => { + Scheduler.unstable_yieldValue('Init A'); + return fakeImport(ChildA); + }); + const LazyChildB = lazy(() => { + Scheduler.unstable_yieldValue('Init B'); + return fakeImport(ChildB); + }); + const LazyChildA2 = lazy(() => { + Scheduler.unstable_yieldValue('Init A2'); + return fakeImport(ChildA); + }); + let resolveB2; + const LazyChildB2 = lazy(() => { + Scheduler.unstable_yieldValue('Init B2'); + return new Promise(r => { + resolveB2 = r; + }); + }); + + function Parent({swap}) { + return ( + }> + }> + {swap + ? [ + , + , + ] + : [, ]} + + + ); + } + + const root = ReactTestRenderer.create(, { + unstable_isConcurrent: true, + }); + + expect(Scheduler).toFlushAndYield(['Init A', 'Init B', 'Loading...']); + expect(root).not.toMatchRenderedOutput('AB'); + + await LazyChildA; + await LazyChildB; + + expect(Scheduler).toFlushAndYield([ + 'A', + 'B', + 'Did mount: A', + 'Did mount: B', + ]); + expect(root).toMatchRenderedOutput('AB'); + + // Swap the position of A and B + root.update(); + expect(Scheduler).toFlushAndYield(['Init B2', 'Loading...']); + jest.runAllTimers(); + + // The suspense boundary should've triggered now. + expect(root).toMatchRenderedOutput('Loading...'); + await resolveB2({default: ChildB}); + + // We need to flush to trigger the second one to load. + expect(Scheduler).toFlushAndYield(['Init A2']); + await LazyChildA2; + + expect(Scheduler).toFlushAndYield([ + 'b', + 'a', + 'Did update: b', + 'Did update: a', + ]); + expect(root).toMatchRenderedOutput('ba'); + }); + + // @gate enableLazyElements + it('mount and reorder lazy types (legacy mode)', async () => { + class Child extends React.Component { + componentDidMount() { + Scheduler.unstable_yieldValue('Did mount: ' + this.props.label); + } + componentDidUpdate() { + Scheduler.unstable_yieldValue('Did update: ' + this.props.label); + } + render() { + return ; + } + } + + function ChildA({lowerCase}) { + return ; + } + + function ChildB({lowerCase}) { + return ; + } + + const LazyChildA = lazy(() => { + Scheduler.unstable_yieldValue('Init A'); + return fakeImport(ChildA); + }); + const LazyChildB = lazy(() => { + Scheduler.unstable_yieldValue('Init B'); + return fakeImport(ChildB); + }); + const LazyChildA2 = lazy(() => { + Scheduler.unstable_yieldValue('Init A2'); + return fakeImport(ChildA); + }); + const LazyChildB2 = lazy(() => { + Scheduler.unstable_yieldValue('Init B2'); + return fakeImport(ChildB); + }); + + function Parent({swap}) { + return ( + }> + }> + {swap + ? [ + , + , + ] + : [, ]} + + + ); + } + + const root = ReactTestRenderer.create(, { + unstable_isConcurrent: false, + }); + + expect(Scheduler).toHaveYielded(['Init A', 'Init B', 'Loading...']); + expect(root).not.toMatchRenderedOutput('AB'); + + await LazyChildA; + await LazyChildB; + + expect(Scheduler).toFlushAndYield([ + 'A', + 'B', + 'Did mount: A', + 'Did mount: B', + ]); + expect(root).toMatchRenderedOutput('AB'); + + // Swap the position of A and B + root.update(); + expect(Scheduler).toHaveYielded(['Init B2', 'Loading...']); + await LazyChildB2; + // We need to flush to trigger the second one to load. + expect(Scheduler).toFlushAndYield(['Init A2']); + await LazyChildA2; + + expect(Scheduler).toFlushAndYield([ + 'b', + 'a', + 'Did update: b', + 'Did update: a', + ]); + expect(root).toMatchRenderedOutput('ba'); + }); + // @gate enableLazyElements it('mount and reorder lazy elements', async () => { class Child extends React.Component { @@ -1343,4 +1529,80 @@ describe('ReactLazy', () => { ]); expect(root).toMatchRenderedOutput('ba'); }); + + // @gate enableLazyElements + it('mount and reorder lazy elements (legacy mode)', async () => { + class Child extends React.Component { + componentDidMount() { + Scheduler.unstable_yieldValue('Did mount: ' + this.props.label); + } + componentDidUpdate() { + Scheduler.unstable_yieldValue('Did update: ' + this.props.label); + } + render() { + return ; + } + } + + const lazyChildA = lazy(() => { + Scheduler.unstable_yieldValue('Init A'); + return fakeImport(); + }); + const lazyChildB = lazy(() => { + Scheduler.unstable_yieldValue('Init B'); + return fakeImport(); + }); + const lazyChildA2 = lazy(() => { + Scheduler.unstable_yieldValue('Init A2'); + return fakeImport(); + }); + const lazyChildB2 = lazy(() => { + Scheduler.unstable_yieldValue('Init B2'); + return fakeImport(); + }); + + function Parent({swap}) { + return ( + }> + {swap ? [lazyChildB2, lazyChildA2] : [lazyChildA, lazyChildB]} + + ); + } + + const root = ReactTestRenderer.create(, { + unstable_isConcurrent: false, + }); + + expect(Scheduler).toHaveYielded(['Init A', 'Loading...']); + expect(root).not.toMatchRenderedOutput('AB'); + + await lazyChildA; + // We need to flush to trigger the B to load. + expect(Scheduler).toFlushAndYield(['Init B']); + await lazyChildB; + + expect(Scheduler).toFlushAndYield([ + 'A', + 'B', + 'Did mount: A', + 'Did mount: B', + ]); + expect(root).toMatchRenderedOutput('AB'); + + // Swap the position of A and B + root.update(); + expect(Scheduler).toHaveYielded(['Init B2', 'Loading...']); + await lazyChildB2; + // We need to flush to trigger the second one to load. + expect(Scheduler).toFlushAndYield(['Init A2']); + await lazyChildA2; + + expect(Scheduler).toFlushAndYield([ + 'b', + 'a', + 'Did update: b', + 'Did update: a', + ]); + expect(root).toMatchRenderedOutput('ba'); + }); }); From a2a025537d5b4e1fd249c0a83217b04092527069 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 1 Dec 2020 10:33:32 -0500 Subject: [PATCH 4/6] Fixed invalid DevTools work tags (#20362) * Fixed invalid DevTools work tags Work tags changed recently (PR #13902) but we didn't bump React versions. This meant that DevTools has valid work tags only for master (and FB www sync) but invalid work tags for the latest open source releases. To fix this, I incremneted React's version in Git (without an actual release) and added a new fork to the work tags detection branch. This commit also adds tags for the experimental Scope and Fundamental APIs to DevTools so component names will at least display correctly. Technically these new APIs were first introduced to experimental builds ~16.9 but I didn't add a new branch to the work tags fork because I don't they're used commonly. I've just added them to the 17+ branches. * Removed FundamentalComponent from DevTools tag defs --- package.json | 4 +- packages/react-art/package.json | 4 +- .../src/backend/renderer.js | 71 +++++++++++++++++-- .../src/backend/types.js | 2 + packages/react-dom/package.json | 4 +- packages/react-is/package.json | 2 +- packages/react-reconciler/package.json | 2 +- packages/react-test-renderer/package.json | 6 +- packages/react/package.json | 2 +- packages/shared/ReactVersion.js | 6 +- 10 files changed, 84 insertions(+), 19 deletions(-) diff --git a/package.json b/package.json index eb95ebce230..0d50364ef51 100644 --- a/package.json +++ b/package.json @@ -106,8 +106,8 @@ }, "scripts": { "build": "node ./scripts/rollup/build.js", - "build-for-devtools-dev": "cross-env RELEASE_CHANNEL=experimental yarn build react/index,react-dom/index,react-is,react-debug-tools,scheduler,react-test-renderer,react-refresh --type=NODE_DEV", - "build-for-devtools-prod": "cross-env RELEASE_CHANNEL=experimental yarn build react/index,react-dom/index,react-is,react-debug-tools,scheduler,react-test-renderer,react-refresh --type=NODE_PROD", + "build-for-devtools-dev": "cross-env RELEASE_CHANNEL=experimental yarn build react/index,react-dom,react-is,react-debug-tools,scheduler,react-test-renderer,react-refresh --type=NODE_DEV", + "build-for-devtools-prod": "cross-env RELEASE_CHANNEL=experimental yarn build react/index,react-dom,react-is,react-debug-tools,scheduler,react-test-renderer,react-refresh --type=NODE_PROD", "linc": "node ./scripts/tasks/linc.js", "lint": "node ./scripts/tasks/eslint.js", "lint-build": "node ./scripts/rollup/validate/index.js", diff --git a/packages/react-art/package.json b/packages/react-art/package.json index 194a9697e0a..ae977717034 100644 --- a/packages/react-art/package.json +++ b/packages/react-art/package.json @@ -1,7 +1,7 @@ { "name": "react-art", "description": "React ART is a JavaScript library for drawing vector graphics using React. It provides declarative and reactive bindings to the ART library. Using the same declarative API you can render the output to either Canvas, SVG or VML (IE8).", - "version": "17.0.1", + "version": "17.0.2", "main": "index.js", "repository": { "type": "git", @@ -29,7 +29,7 @@ "scheduler": "^0.20.1" }, "peerDependencies": { - "react": "17.0.1" + "react": "17.0.2" }, "files": [ "LICENSE", diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 1245c254c55..f8e0f07104a 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -7,7 +7,7 @@ * @flow */ -import {gte} from 'semver'; +import {gt, gte} from 'semver'; import { ComponentFilterDisplayName, ComponentFilterElementType, @@ -166,8 +166,10 @@ export function getInternalReactConstants( // ********************************************************** // The section below is copied from files in React repo. // Keep it in sync, and add version guards if it changes. - if (gte(version, '17.0.0-alpha')) { - // TODO (Offscreen) Update the version number above to reflect the first Offscreen alpha/beta release. + // + // TODO Update the gt() check below to be gte() whichever the next version number is. + // Currently the version in Git is 17.0.2 (but that version has not been/may not end up being released). + if (gt(version, '17.0.1')) { ReactTypeOfWork = { ClassComponent: 1, ContextConsumer: 9, @@ -185,10 +187,41 @@ export function getInternalReactConstants( IncompleteClassComponent: 17, IndeterminateComponent: 2, LazyComponent: 16, + LegacyHiddenComponent: 23, MemoComponent: 14, Mode: 8, OffscreenComponent: 22, // Experimental Profiler: 12, + ScopeComponent: 21, // Experimental + SimpleMemoComponent: 15, + SuspenseComponent: 13, + SuspenseListComponent: 19, // Experimental + YieldComponent: -1, // Removed + }; + } else if (gte(version, '17.0.0-alpha')) { + ReactTypeOfWork = { + ClassComponent: 1, + ContextConsumer: 9, + ContextProvider: 10, + CoroutineComponent: -1, // Removed + CoroutineHandlerPhase: -1, // Removed + DehydratedSuspenseComponent: 18, // Behind a flag + ForwardRef: 11, + Fragment: 7, + FunctionComponent: 0, + HostComponent: 5, + HostPortal: 4, + HostRoot: 3, + HostText: 6, + IncompleteClassComponent: 17, + IndeterminateComponent: 2, + LazyComponent: 16, + LegacyHiddenComponent: 24, + MemoComponent: 14, + Mode: 8, + OffscreenComponent: 23, // Experimental + Profiler: 12, + ScopeComponent: 21, // Experimental SimpleMemoComponent: 15, SuspenseComponent: 13, SuspenseListComponent: 19, // Experimental @@ -212,10 +245,12 @@ export function getInternalReactConstants( IncompleteClassComponent: 17, IndeterminateComponent: 2, LazyComponent: 16, + LegacyHiddenComponent: -1, MemoComponent: 14, Mode: 8, OffscreenComponent: -1, // Experimental Profiler: 12, + ScopeComponent: -1, // Experimental SimpleMemoComponent: 15, SuspenseComponent: 13, SuspenseListComponent: 19, // Experimental @@ -239,10 +274,12 @@ export function getInternalReactConstants( IncompleteClassComponent: -1, // Doesn't exist yet IndeterminateComponent: 4, LazyComponent: -1, // Doesn't exist yet + LegacyHiddenComponent: -1, MemoComponent: -1, // Doesn't exist yet Mode: 10, OffscreenComponent: -1, // Experimental Profiler: 15, + ScopeComponent: -1, // Experimental SimpleMemoComponent: -1, // Doesn't exist yet SuspenseComponent: 16, SuspenseListComponent: -1, // Doesn't exist yet @@ -266,10 +303,12 @@ export function getInternalReactConstants( IncompleteClassComponent: -1, // Doesn't exist yet IndeterminateComponent: 0, LazyComponent: -1, // Doesn't exist yet + LegacyHiddenComponent: -1, MemoComponent: -1, // Doesn't exist yet Mode: 11, OffscreenComponent: -1, // Experimental Profiler: 15, + ScopeComponent: -1, // Experimental SimpleMemoComponent: -1, // Doesn't exist yet SuspenseComponent: 16, SuspenseListComponent: -1, // Doesn't exist yet @@ -301,7 +340,11 @@ export function getInternalReactConstants( HostPortal, HostText, Fragment, + LazyComponent, + LegacyHiddenComponent, MemoComponent, + OffscreenComponent, + ScopeComponent, SimpleMemoComponent, SuspenseComponent, SuspenseListComponent, @@ -354,11 +397,22 @@ export function getInternalReactConstants( case HostText: case Fragment: return null; + case LazyComponent: + // This display name will not be user visible. + // Once a Lazy component loads its inner component, React replaces the tag and type. + // This display name will only show up in console logs when DevTools DEBUG mode is on. + return 'Lazy'; case MemoComponent: case SimpleMemoComponent: return getDisplayName(resolvedType, 'Anonymous'); case SuspenseComponent: return 'Suspense'; + case LegacyHiddenComponent: + return 'LegacyHidden'; + case OffscreenComponent: + return 'Offscreen'; + case ScopeComponent: + return 'Scope'; case SuspenseListComponent: return 'SuspenseList'; default: @@ -493,10 +547,14 @@ export function attach( const debug = (name: string, fiber: Fiber, parentFiber: ?Fiber): void => { if (__DEBUG__) { - const displayName = getDisplayNameForFiber(fiber) || 'null'; + const displayName = + fiber.tag + ':' + (getDisplayNameForFiber(fiber) || 'null'); const id = getFiberID(fiber); - const parentDisplayName = - (parentFiber != null && getDisplayNameForFiber(parentFiber)) || 'null'; + const parentDisplayName = parentFiber + ? parentFiber.tag + + ':' + + (getDisplayNameForFiber(parentFiber) || 'null') + : ''; const parentID = parentFiber ? getFiberID(parentFiber) : ''; // NOTE: calling getFiberID or getPrimaryFiber is unsafe here // because it will put them in the map. For now, we'll omit them. @@ -1207,6 +1265,7 @@ export function attach( return; } const id = getFiberID(primaryFiber); + if (isRoot) { // Roots must be removed only after all children (pending and simulated) have been removed. // So we track it separately. diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 4ec480aceda..d87c2fdf97b 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -42,10 +42,12 @@ export type WorkTagMap = {| IncompleteClassComponent: WorkTag, IndeterminateComponent: WorkTag, LazyComponent: WorkTag, + LegacyHiddenComponent: WorkTag, MemoComponent: WorkTag, Mode: WorkTag, OffscreenComponent: WorkTag, Profiler: WorkTag, + ScopeComponent: WorkTag, SimpleMemoComponent: WorkTag, SuspenseComponent: WorkTag, SuspenseListComponent: WorkTag, diff --git a/packages/react-dom/package.json b/packages/react-dom/package.json index f512ed9f516..5e8991e8987 100644 --- a/packages/react-dom/package.json +++ b/packages/react-dom/package.json @@ -1,6 +1,6 @@ { "name": "react-dom", - "version": "17.0.1", + "version": "17.0.2", "description": "React package for working with the DOM.", "main": "index.js", "repository": { @@ -22,7 +22,7 @@ "scheduler": "^0.20.1" }, "peerDependencies": { - "react": "17.0.1" + "react": "17.0.2" }, "files": [ "LICENSE", diff --git a/packages/react-is/package.json b/packages/react-is/package.json index 1687b93fae9..1a6567dcff5 100644 --- a/packages/react-is/package.json +++ b/packages/react-is/package.json @@ -1,6 +1,6 @@ { "name": "react-is", - "version": "17.0.1", + "version": "17.0.2", "description": "Brand checking of React Elements.", "main": "index.js", "repository": { diff --git a/packages/react-reconciler/package.json b/packages/react-reconciler/package.json index 858a7190a6e..a6fd3a05a96 100644 --- a/packages/react-reconciler/package.json +++ b/packages/react-reconciler/package.json @@ -26,7 +26,7 @@ "node": ">=0.10.0" }, "peerDependencies": { - "react": "^17.0.1" + "react": "^17.0.2" }, "dependencies": { "loose-envify": "^1.1.0", diff --git a/packages/react-test-renderer/package.json b/packages/react-test-renderer/package.json index c12ce070d8f..ea62201e24b 100644 --- a/packages/react-test-renderer/package.json +++ b/packages/react-test-renderer/package.json @@ -1,6 +1,6 @@ { "name": "react-test-renderer", - "version": "17.0.1", + "version": "17.0.2", "description": "React package for snapshot testing.", "main": "index.js", "repository": { @@ -20,12 +20,12 @@ "homepage": "https://reactjs.org/", "dependencies": { "object-assign": "^4.1.1", - "react-is": "^17.0.1", + "react-is": "^17.0.2", "react-shallow-renderer": "^16.13.1", "scheduler": "^0.20.1" }, "peerDependencies": { - "react": "17.0.1" + "react": "17.0.2" }, "files": [ "LICENSE", diff --git a/packages/react/package.json b/packages/react/package.json index 993a45af3bc..0dd2c35ee62 100644 --- a/packages/react/package.json +++ b/packages/react/package.json @@ -4,7 +4,7 @@ "keywords": [ "react" ], - "version": "17.0.1", + "version": "17.0.2", "homepage": "https://reactjs.org/", "bugs": "https://github.com/facebook/react/issues", "license": "MIT", diff --git a/packages/shared/ReactVersion.js b/packages/shared/ReactVersion.js index b042a00d114..0404e397227 100644 --- a/packages/shared/ReactVersion.js +++ b/packages/shared/ReactVersion.js @@ -6,4 +6,8 @@ */ // TODO: this is special because it gets imported during build. -export default '17.0.1'; +// +// TODO: 17.0.2 has not been released to NPM; +// It exists as a placeholder so that DevTools can support work tag changes between releases. +// When we next publish a release (either 17.0.2 or 17.1.0), update the matching TODO in backend/renderer.js +export default '17.0.2'; From ad6f3d5c55a79e8a44798aad36118e73de3a64f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 1 Dec 2020 11:01:16 -0500 Subject: [PATCH 5/6] Ignore node_modules when printing warnings (#20363) This now finds acorn and fails to extract warnings from it. But also, this seems slow. --- scripts/print-warnings/print-warnings.js | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/print-warnings/print-warnings.js b/scripts/print-warnings/print-warnings.js index 163aacf00ac..9bdd6543d2a 100644 --- a/scripts/print-warnings/print-warnings.js +++ b/scripts/print-warnings/print-warnings.js @@ -90,6 +90,7 @@ gs([ '!packages/react-devtools*/**/*.js', '!**/__tests__/**/*.js', '!**/__mocks__/**/*.js', + '!**/node_modules/**/*.js', ]).pipe( through.obj(transform, cb => { process.stdout.write( From 39734ee8248814210ab7bd3a88e88bc06066dd25 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 23 Nov 2020 02:18:20 +0000 Subject: [PATCH 6/6] Failing test for Client reconciliation --- .../src/__tests__/ReactFlightDOM-test.js | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/packages/react-transport-dom-webpack/src/__tests__/ReactFlightDOM-test.js b/packages/react-transport-dom-webpack/src/__tests__/ReactFlightDOM-test.js index e437c7b20bc..4ff3ff20341 100644 --- a/packages/react-transport-dom-webpack/src/__tests__/ReactFlightDOM-test.js +++ b/packages/react-transport-dom-webpack/src/__tests__/ReactFlightDOM-test.js @@ -440,4 +440,92 @@ describe('ReactFlightDOM', () => { '

Game over

', // TODO: should not have message in prod. ); }); + + // @gate experimental + it('should preserve state of client components on refetch', async () => { + const {Suspense} = React; + + // Client + + function Page({response}) { + return response.readRoot(); + } + + function Input() { + return ; + } + + const InputClient = moduleReference(Input); + + // Server + + function App({color}) { + // Verify both DOM and Client children. + return ( +
+ + +
+ ); + } + + const container = document.createElement('div'); + const root = ReactDOM.unstable_createRoot(container); + + const stream1 = getTestStream(); + ReactTransportDOMServer.pipeToNodeWritable( + , + stream1.writable, + webpackMap, + ); + const response1 = ReactTransportDOMClient.createFromReadableStream( + stream1.readable, + ); + await act(async () => { + root.render( + (loading)

}> + + , + ); + }); + expect(container.children.length).toBe(1); + expect(container.children[0].tagName).toBe('DIV'); + expect(container.children[0].style.color).toBe('red'); + + // Change the DOM state for both inputs. + const inputA = container.children[0].children[0]; + expect(inputA.tagName).toBe('INPUT'); + inputA.value = 'hello'; + const inputB = container.children[0].children[1]; + expect(inputB.tagName).toBe('INPUT'); + inputB.value = 'goodbye'; + + const stream2 = getTestStream(); + ReactTransportDOMServer.pipeToNodeWritable( + , + stream2.writable, + webpackMap, + ); + const response2 = ReactTransportDOMClient.createFromReadableStream( + stream2.readable, + ); + await act(async () => { + root.render( + (loading)

}> + + , + ); + }); + expect(container.children.length).toBe(1); + expect(container.children[0].tagName).toBe('DIV'); + expect(container.children[0].style.color).toBe('blue'); + + // Verify we didn't destroy the DOM for either input. + expect(inputA === container.children[0].children[0]).toBe(true); + expect(inputA.tagName).toBe('INPUT'); + expect(inputA.value).toBe('hello'); + expect(inputB === container.children[0].children[1]).toBe(true); + expect(inputB.tagName).toBe('INPUT'); + expect(inputB.value).toBe('goodbye'); + }); });