From 0ca8420f9d051285c452a590ac4a4c9476406bef Mon Sep 17 00:00:00 2001 From: "Sebastian \"Sebbie\" Silbermann" Date: Sun, 4 May 2025 13:47:32 +0200 Subject: [PATCH 01/31] [Flight] Use valid CSS selectors in `useId` format (#33099) --- .../react-client/src/__tests__/ReactFlight-test.js | 12 ++++++------ packages/react-server/src/ReactFlightHooks.js | 8 +++++++- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index 4449207e88..b954f32ecd 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -1957,8 +1957,8 @@ describe('ReactFlight', () => { }); expect(ReactNoop).toMatchRenderedOutput( <> -
-
+
+
, ); }); @@ -1981,8 +1981,8 @@ describe('ReactFlight', () => { }); expect(ReactNoop).toMatchRenderedOutput( <> -
-
+
+
, ); }); @@ -2021,8 +2021,8 @@ describe('ReactFlight', () => { assertLog(['ClientDoubler']); expect(ReactNoop).toMatchRenderedOutput( <> -
:S1:
-
:S1:
+
«S1»
+
«S1»
, ); }); diff --git a/packages/react-server/src/ReactFlightHooks.js b/packages/react-server/src/ReactFlightHooks.js index bcf36d4c9b..442f6207a9 100644 --- a/packages/react-server/src/ReactFlightHooks.js +++ b/packages/react-server/src/ReactFlightHooks.js @@ -120,7 +120,13 @@ function useId(): string { } const id = currentRequest.identifierCount++; // use 'S' for Flight components to distinguish from 'R' and 'r' in Fizz/Client - return ':' + currentRequest.identifierPrefix + 'S' + id.toString(32) + ':'; + return ( + '\u00AB' + + currentRequest.identifierPrefix + + 'S' + + id.toString(32) + + '\u00BB' + ); } function use(usable: Usable): T { From 3ec88e797f7352c87cb79292b02fdfc53050133f Mon Sep 17 00:00:00 2001 From: Stephen Zhou <38493346+hyoban@users.noreply.github.com> Date: Mon, 5 May 2025 23:37:06 +0800 Subject: [PATCH 02/31] [eslint-plugin-react-hooks] update doc url for rules of hooks (#33118) --- packages/eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts b/packages/eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts index 6be40df532..ac7d0f3a06 100644 --- a/packages/eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts +++ b/packages/eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts @@ -128,7 +128,7 @@ const rule = { docs: { description: 'enforces the Rules of Hooks', recommended: true, - url: 'https://reactjs.org/docs/hooks-rules.html', + url: 'https://react.dev/reference/rules/rules-of-hooks', }, }, create(context: Rule.RuleContext) { From 52ea641449570bbc32eb90fb1a76740249b6bcf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 5 May 2025 11:37:39 -0400 Subject: [PATCH 03/31] [Flight] Don't increase serializedSize for every recursive pass (#33123) I noticed that we increase this in the recursive part of the algorithm. This would mean that we'd count a key more than once if it has Server Components inside it recursively resolving. This moves it out to where we enter from toJSON. Which is called once per JSON entry (and therefore once per key). --- packages/react-server/src/ReactFlightServer.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index aefcf5f6ee..0fcd8af7cb 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -2302,6 +2302,9 @@ function renderModel( key: string, value: ReactClientValue, ): ReactJSONValue { + // First time we're serializing the key, we should add it to the size. + serializedSize += key.length; + const prevKeyPath = task.keyPath; const prevImplicitSlot = task.implicitSlot; try { @@ -2416,8 +2419,6 @@ function renderModelDestructive( // Set the currently rendering model task.model = value; - serializedSize += parentPropertyName.length; - // Special Symbol, that's very common. if (value === REACT_ELEMENT_TYPE) { return '$'; From 0c1575cee8a78dd097edcafc307522ad000e372c Mon Sep 17 00:00:00 2001 From: mofeiZ <34200447+mofeiZ@users.noreply.github.com> Date: Mon, 5 May 2025 11:45:58 -0400 Subject: [PATCH 04/31] [compiler][bugfix] Bail out when a memo block declares hoisted fns (#32765) Note that bailing out adds false positives for hoisted functions whose only references are within other functions. For example, this rewrite would be safe. ```js // source program function foo() { return bar(); } function bar() { return 42; } // compiler output let bar; if (/* deps changed */) { function foo() { return bar(); } bar = function bar() { return 42; } } ``` These false positives are difficult to detect because any maybe-call of foo before the definition of bar would be invalid. Instead of bailing out, we should rewrite hoisted function declarations to the following form. ```js let bar$0; if (/* deps changed */) { // All references within the declaring memo block // or before the function declaration should use // the original identifier `bar` function foo() { return bar(); } function bar() { return 42; } bar$0 = bar; } // All references after the declaring memo block // or after the function declaration should use // the rewritten declaration `bar$0` ``` --- .../ReactiveScopes/PruneHoistedContexts.ts | 96 ++++++++++++++++++- .../bug-functiondecl-hoisting.expect.md | 79 --------------- ...error.todo-functiondecl-hoisting.expect.md | 43 +++++++++ ...x => error.todo-functiondecl-hoisting.tsx} | 0 ...todo-valid-functiondecl-hoisting.expect.md | 46 +++++++++ ...error.todo-valid-functiondecl-hoisting.tsx | 25 +++++ 6 files changed, 206 insertions(+), 83 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-functiondecl-hoisting.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-functiondecl-hoisting.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{bug-functiondecl-hoisting.tsx => error.todo-functiondecl-hoisting.tsx} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-valid-functiondecl-hoisting.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-valid-functiondecl-hoisting.tsx diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts index 66b1e1ce15..ae3ff122a2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts @@ -5,10 +5,13 @@ * LICENSE file in the root directory of this source tree. */ +import {CompilerError} from '..'; import { convertHoistedLValueKind, IdentifierId, + InstructionId, InstructionKind, + Place, ReactiveFunction, ReactiveInstruction, ReactiveScopeBlock, @@ -24,15 +27,38 @@ import { /* * Prunes DeclareContexts lowered for HoistedConsts, and transforms any references back to its * original instruction kind. + * + * Also detects and bails out on context variables which are: + * - function declarations, which are hoisted by JS engines to the nearest block scope + * - referenced before they are defined (i.e. having a `DeclareContext HoistedConst`) + * - declared + * + * This is because React Compiler converts a `function foo()` function declaration to + * 1. a `let foo;` declaration before reactive memo blocks + * 2. a `foo = function foo() {}` assignment within the block + * + * This means references before the assignment are invalid (see fixture + * error.todo-functiondecl-hoisting) */ export function pruneHoistedContexts(fn: ReactiveFunction): void { visitReactiveFunction(fn, new Visitor(), { activeScopes: empty(), + uninitialized: new Map(), }); } type VisitorState = { activeScopes: Stack>; + uninitialized: Map< + IdentifierId, + | { + kind: 'unknown-kind'; + } + | { + kind: 'func'; + definition: Place | null; + } + >; }; class Visitor extends ReactiveFunctionTransform { @@ -40,15 +66,39 @@ class Visitor extends ReactiveFunctionTransform { state.activeScopes = state.activeScopes.push( new Set(scope.scope.declarations.keys()), ); + /** + * Add declared but not initialized / assigned variables. This may include + * function declarations that escape the memo block. + */ + for (const decl of scope.scope.declarations.values()) { + state.uninitialized.set(decl.identifier.id, {kind: 'unknown-kind'}); + } this.traverseScope(scope, state); state.activeScopes.pop(); + for (const decl of scope.scope.declarations.values()) { + state.uninitialized.delete(decl.identifier.id); + } + } + override visitPlace( + _id: InstructionId, + place: Place, + state: VisitorState, + ): void { + const maybeHoistedFn = state.uninitialized.get(place.identifier.id); + if ( + maybeHoistedFn?.kind === 'func' && + maybeHoistedFn.definition !== place + ) { + CompilerError.throwTodo({ + reason: '[PruneHoistedContexts] Rewrite hoisted function references', + loc: place.loc, + }); + } } override transformInstruction( instruction: ReactiveInstruction, state: VisitorState, ): Transformed { - this.visitInstruction(instruction, state); - /** * Remove hoisted declarations to preserve TDZ */ @@ -57,6 +107,18 @@ class Visitor extends ReactiveFunctionTransform { instruction.value.lvalue.kind, ); if (maybeNonHoisted != null) { + if ( + maybeNonHoisted === InstructionKind.Function && + state.uninitialized.has(instruction.value.lvalue.place.identifier.id) + ) { + state.uninitialized.set( + instruction.value.lvalue.place.identifier.id, + { + kind: 'func', + definition: null, + }, + ); + } return {kind: 'remove'}; } } @@ -65,7 +127,7 @@ class Visitor extends ReactiveFunctionTransform { instruction.value.lvalue.kind !== InstructionKind.Reassign ) { /** - * Rewrite StoreContexts let/const/functions that will be pre-declared in + * Rewrite StoreContexts let/const that will be pre-declared in * codegen to reassignments. */ const lvalueId = instruction.value.lvalue.place.identifier.id; @@ -73,10 +135,36 @@ class Visitor extends ReactiveFunctionTransform { scope.has(lvalueId), ); if (isDeclaredByScope) { - instruction.value.lvalue.kind = InstructionKind.Reassign; + if ( + instruction.value.lvalue.kind === InstructionKind.Let || + instruction.value.lvalue.kind === InstructionKind.Const + ) { + instruction.value.lvalue.kind = InstructionKind.Reassign; + } else if (instruction.value.lvalue.kind === InstructionKind.Function) { + const maybeHoistedFn = state.uninitialized.get(lvalueId); + if (maybeHoistedFn != null) { + CompilerError.invariant(maybeHoistedFn.kind === 'func', { + reason: '[PruneHoistedContexts] Unexpected hoisted function', + loc: instruction.loc, + }); + maybeHoistedFn.definition = instruction.value.lvalue.place; + /** + * References to hoisted functions are now "safe" as variable assignments + * have finished. + */ + state.uninitialized.delete(lvalueId); + } + } else { + CompilerError.throwTodo({ + reason: '[PruneHoistedContexts] Unexpected kind', + description: `(${instruction.value.lvalue.kind})`, + loc: instruction.loc, + }); + } } } + this.visitInstruction(instruction, state); return {kind: 'keep'}; } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-functiondecl-hoisting.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-functiondecl-hoisting.expect.md deleted file mode 100644 index f8712ed728..0000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-functiondecl-hoisting.expect.md +++ /dev/null @@ -1,79 +0,0 @@ - -## Input - -```javascript -import {Stringify} from 'shared-runtime'; - -/** - * Fixture currently fails with - * Found differences in evaluator results - * Non-forget (expected): - * (kind: ok)
{"result":{"value":2},"fn":{"kind":"Function","result":{"value":2}},"shouldInvokeFns":true}
- * Forget: - * (kind: exception) bar is not a function - */ -function Foo({value}) { - const result = bar(); - function bar() { - return {value}; - } - return ; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Foo, - params: [{value: 2}], -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; -import { Stringify } from "shared-runtime"; - -/** - * Fixture currently fails with - * Found differences in evaluator results - * Non-forget (expected): - * (kind: ok)
{"result":{"value":2},"fn":{"kind":"Function","result":{"value":2}},"shouldInvokeFns":true}
- * Forget: - * (kind: exception) bar is not a function - */ -function Foo(t0) { - const $ = _c(6); - const { value } = t0; - let bar; - let result; - if ($[0] !== value) { - result = bar(); - bar = function bar() { - return { value }; - }; - $[0] = value; - $[1] = bar; - $[2] = result; - } else { - bar = $[1]; - result = $[2]; - } - let t1; - if ($[3] !== bar || $[4] !== result) { - t1 = ; - $[3] = bar; - $[4] = result; - $[5] = t1; - } else { - t1 = $[5]; - } - return t1; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Foo, - params: [{ value: 2 }], -}; - -``` - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-functiondecl-hoisting.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-functiondecl-hoisting.expect.md new file mode 100644 index 0000000000..6e2310bd10 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-functiondecl-hoisting.expect.md @@ -0,0 +1,43 @@ + +## Input + +```javascript +import {Stringify} from 'shared-runtime'; + +/** + * Fixture currently fails with + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok)
{"result":{"value":2},"fn":{"kind":"Function","result":{"value":2}},"shouldInvokeFns":true}
+ * Forget: + * (kind: exception) bar is not a function + */ +function Foo({value}) { + const result = bar(); + function bar() { + return {value}; + } + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{value: 2}], +}; + +``` + + +## Error + +``` + 10 | */ + 11 | function Foo({value}) { +> 12 | const result = bar(); + | ^^^ Todo: [PruneHoistedContexts] Rewrite hoisted function references (12:12) + 13 | function bar() { + 14 | return {value}; + 15 | } +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-functiondecl-hoisting.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-functiondecl-hoisting.tsx similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-functiondecl-hoisting.tsx rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-functiondecl-hoisting.tsx diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-valid-functiondecl-hoisting.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-valid-functiondecl-hoisting.expect.md new file mode 100644 index 0000000000..fb8988c8f8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-valid-functiondecl-hoisting.expect.md @@ -0,0 +1,46 @@ + +## Input + +```javascript +import {Stringify} from 'shared-runtime'; +/** + * Also see error.todo-functiondecl-hoisting.tsx which shows *invalid* + * compilation cases. + * + * This bailout specifically is a false positive for since this function's only + * reference-before-definition are within other functions which are not invoked. + */ +function Foo() { + 'use memo'; + + function foo() { + return bar(); + } + function bar() { + return 42; + } + + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [], +}; + +``` + + +## Error + +``` + 13 | return bar(); + 14 | } +> 15 | function bar() { + | ^^^ Todo: [PruneHoistedContexts] Rewrite hoisted function references (15:15) + 16 | return 42; + 17 | } + 18 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-valid-functiondecl-hoisting.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-valid-functiondecl-hoisting.tsx new file mode 100644 index 0000000000..063492b89c --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-valid-functiondecl-hoisting.tsx @@ -0,0 +1,25 @@ +import {Stringify} from 'shared-runtime'; +/** + * Also see error.todo-functiondecl-hoisting.tsx which shows *invalid* + * compilation cases. + * + * This bailout specifically is a false positive for since this function's only + * reference-before-definition are within other functions which are not invoked. + */ +function Foo() { + 'use memo'; + + function foo() { + return bar(); + } + function bar() { + return 42; + } + + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [], +}; From c129c2424b662a371865a0145c562a1cf934b023 Mon Sep 17 00:00:00 2001 From: mofeiZ <34200447+mofeiZ@users.noreply.github.com> Date: Mon, 5 May 2025 11:52:45 -0400 Subject: [PATCH 05/31] [compiler][repro] Nested fbt test fixture (#32779) Ideally we should detect and bail out on this case to avoid babel build failures. --- .../error.todo-fbt-param-nested-fbt.expect.md | 56 +++++++++++++++++++ .../fbt/error.todo-fbt-param-nested-fbt.js | 38 +++++++++++++ 2 files changed, 94 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/error.todo-fbt-param-nested-fbt.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/error.todo-fbt-param-nested-fbt.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/error.todo-fbt-param-nested-fbt.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/error.todo-fbt-param-nested-fbt.expect.md new file mode 100644 index 0000000000..eb8073282e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/error.todo-fbt-param-nested-fbt.expect.md @@ -0,0 +1,56 @@ + +## Input + +```javascript +import fbt from 'fbt'; +import {Stringify} from 'shared-runtime'; + +/** + * MemoizeFbtAndMacroOperands needs to account for nested fbt calls. + * Expected fixture `fbt-param-call-arguments` to succeed but it failed with error: + * /fbt-param-call-arguments.ts: Line 19 Column 11: fbt: unsupported babel node: Identifier + * --- + * t3 + * --- + */ +function Component({firstname, lastname}) { + 'use memo'; + return ( + + {fbt( + [ + 'Name: ', + fbt.param('firstname', ), + ', ', + fbt.param( + 'lastname', + + {fbt('(inner fbt)', 'Inner fbt value')} + + ), + ], + 'Name' + )} + + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{firstname: 'first', lastname: 'last'}], + sequentialRenders: [{firstname: 'first', lastname: 'last'}], +}; + +``` + + +## Error + +``` +Line 19 Column 11: fbt: unsupported babel node: Identifier +--- +t3 +--- +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/error.todo-fbt-param-nested-fbt.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/error.todo-fbt-param-nested-fbt.js new file mode 100644 index 0000000000..14e3278e39 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/error.todo-fbt-param-nested-fbt.js @@ -0,0 +1,38 @@ +import fbt from 'fbt'; +import {Stringify} from 'shared-runtime'; + +/** + * MemoizeFbtAndMacroOperands needs to account for nested fbt calls. + * Expected fixture `fbt-param-call-arguments` to succeed but it failed with error: + * /fbt-param-call-arguments.ts: Line 19 Column 11: fbt: unsupported babel node: Identifier + * --- + * t3 + * --- + */ +function Component({firstname, lastname}) { + 'use memo'; + return ( + + {fbt( + [ + 'Name: ', + fbt.param('firstname', ), + ', ', + fbt.param( + 'lastname', + + {fbt('(inner fbt)', 'Inner fbt value')} + + ), + ], + 'Name' + )} + + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{firstname: 'first', lastname: 'last'}], + sequentialRenders: [{firstname: 'first', lastname: 'last'}], +}; From b9cfa0d3083f80bdd11ba76a55aa08fa659b7359 Mon Sep 17 00:00:00 2001 From: "Sebastian \"Sebbie\" Silbermann" Date: Mon, 5 May 2025 18:30:33 +0200 Subject: [PATCH 06/31] [Flight] Prevent serialized size leaking across requests (#33121) --- .../src/__tests__/ReactFlightDOMEdge-test.js | 38 +++++++++++++++++++ .../react-server/src/ReactFlightServer.js | 23 ++++++----- 2 files changed, 49 insertions(+), 12 deletions(-) diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js index 5194913d2c..a4418b7017 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js @@ -692,6 +692,44 @@ describe('ReactFlightDOMEdge', () => { expect(html).toBe(html2); }); + it('regression: should not leak serialized size', async () => { + const MAX_ROW_SIZE = 3200; + // This test case is a bit convoluted and may no longer trigger the original bug. + // Originally, the size of `promisedText` was not cleaned up so the sync portion + // ended up being deferred immediately when we called `renderToReadableStream` again + // i.e. `result2.syncText` became a Lazy element on the second request. + const longText = 'd'.repeat(MAX_ROW_SIZE); + const promisedText = Promise.resolve(longText); + const model = {syncText:

{longText}

, promisedText}; + + const stream = await serverAct(() => + ReactServerDOMServer.renderToReadableStream(model), + ); + + const result = await ReactServerDOMClient.createFromReadableStream(stream, { + serverConsumerManifest: { + moduleMap: null, + moduleLoading: null, + }, + }); + + const stream2 = await serverAct(() => + ReactServerDOMServer.renderToReadableStream(model), + ); + + const result2 = await ReactServerDOMClient.createFromReadableStream( + stream2, + { + serverConsumerManifest: { + moduleMap: null, + moduleLoading: null, + }, + }, + ); + + expect(result2.syncText).toEqual(result.syncText); + }); + it('should be able to serialize any kind of typed array', async () => { const buffer = new Uint8Array([ 123, 4, 10, 5, 100, 255, 244, 45, 56, 67, 43, 124, 67, 89, 100, 20, diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 0fcd8af7cb..98b17cc920 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -3927,18 +3927,9 @@ function emitChunk( return; } // For anything else we need to try to serialize it using JSON. - // We stash the outer parent size so we can restore it when we exit. - const parentSerializedSize = serializedSize; - // We don't reset the serialized size counter from reentry because that indicates that we - // are outlining a model and we actually want to include that size into the parent since - // it will still block the parent row. It only restores to zero at the top of the stack. - try { - // $FlowFixMe[incompatible-type] stringify can return null for undefined but we never do - const json: string = stringify(value, task.toJSON); - emitModelChunk(request, task.id, json); - } finally { - serializedSize = parentSerializedSize; - } + // $FlowFixMe[incompatible-type] stringify can return null for undefined but we never do + const json: string = stringify(value, task.toJSON); + emitModelChunk(request, task.id, json); } function erroredTask(request: Request, task: Task, error: mixed): void { @@ -3976,6 +3967,11 @@ function retryTask(request: Request, task: Task): void { const prevDebugID = debugID; task.status = RENDERING; + // We stash the outer parent size so we can restore it when we exit. + const parentSerializedSize = serializedSize; + // We don't reset the serialized size counter from reentry because that indicates that we + // are outlining a model and we actually want to include that size into the parent since + // it will still block the parent row. It only restores to zero at the top of the stack. try { // Track the root so we know that we have to emit this object even though it // already has an ID. This is needed because we might see this object twice @@ -4087,6 +4083,7 @@ function retryTask(request: Request, task: Task): void { if (__DEV__) { debugID = prevDebugID; } + serializedSize = parentSerializedSize; } } @@ -4099,9 +4096,11 @@ function tryStreamTask(request: Request, task: Task): void { // so that we instead outline the row to get a new debugID if needed. debugID = null; } + const parentSerializedSize = serializedSize; try { emitChunk(request, task, task.model); } finally { + serializedSize = parentSerializedSize; if (__DEV__) { debugID = prevDebugID; } From edf550b67936f2c62534ad5549bf580a4f581bd8 Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Mon, 5 May 2025 13:36:44 -0400 Subject: [PATCH 07/31] Ship enableFabricCompleteRootInCommitPhase (#33064) This was shipped internally. Cleaning up the flag. --- .../src/ReactFiberConfigFabric.js | 10 ++-------- packages/shared/ReactFeatureFlags.js | 5 ----- .../forks/ReactFeatureFlags.native-fb-dynamic.js | 1 - packages/shared/forks/ReactFeatureFlags.native-fb.js | 1 - packages/shared/forks/ReactFeatureFlags.native-oss.js | 1 - .../shared/forks/ReactFeatureFlags.test-renderer.js | 1 - .../forks/ReactFeatureFlags.test-renderer.native-fb.js | 1 - .../forks/ReactFeatureFlags.test-renderer.www.js | 1 - packages/shared/forks/ReactFeatureFlags.www.js | 1 - 9 files changed, 2 insertions(+), 20 deletions(-) diff --git a/packages/react-native-renderer/src/ReactFiberConfigFabric.js b/packages/react-native-renderer/src/ReactFiberConfigFabric.js index 1cf98193ab..afa0a9c218 100644 --- a/packages/react-native-renderer/src/ReactFiberConfigFabric.js +++ b/packages/react-native-renderer/src/ReactFiberConfigFabric.js @@ -63,7 +63,6 @@ import { } from './ReactNativeFiberInspector'; import { - enableFabricCompleteRootInCommitPhase, passChildrenWhenCloningPersistedNodes, enableLazyPublicInstanceInFabric, } from 'shared/ReactFeatureFlags'; @@ -543,19 +542,14 @@ export function finalizeContainerChildren( container: Container, newChildren: ChildSet, ): void { - if (!enableFabricCompleteRootInCommitPhase) { - completeRoot(container.containerTag, newChildren); - } + // Noop - children will be replaced in replaceContainerChildren } export function replaceContainerChildren( container: Container, newChildren: ChildSet, ): void { - // Noop - children will be replaced in finalizeContainerChildren - if (enableFabricCompleteRootInCommitPhase) { - completeRoot(container.containerTag, newChildren); - } + completeRoot(container.containerTag, newChildren); } export {getClosestInstanceFromNode as getInstanceFromNode}; diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 10bd08970f..5237e31231 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -100,11 +100,6 @@ export const enableSuspenseyImages = false; export const enableSrcObject = __EXPERIMENTAL__; -/** - * Switches the Fabric API from doing layout in commit work instead of complete work. - */ -export const enableFabricCompleteRootInCommitPhase = false; - /** * Switches Fiber creation to a simple object instead of a constructor. */ diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js b/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js index f1ced67c44..c2b56602b3 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js @@ -23,7 +23,6 @@ export const enableHiddenSubtreeInsertionEffectCleanup = __VARIANT__; export const enablePersistedModeClonedFlag = __VARIANT__; export const enableShallowPropDiffing = __VARIANT__; export const passChildrenWhenCloningPersistedNodes = __VARIANT__; -export const enableFabricCompleteRootInCommitPhase = __VARIANT__; export const enableSiblingPrerendering = __VARIANT__; export const enableFastAddPropertiesInDiffing = __VARIANT__; export const enableLazyPublicInstanceInFabric = __VARIANT__; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 6bc3f7b1d1..93c3ebff00 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -20,7 +20,6 @@ const dynamicFlags: DynamicExportsType = (dynamicFlagsUntyped: any); // the exports object every time a flag is read. export const { alwaysThrottleRetries, - enableFabricCompleteRootInCommitPhase, enableHiddenSubtreeInsertionEffectCleanup, enableObjectFiber, enablePersistedModeClonedFlag, diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index cb2d97eb70..c1e7cebdae 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -30,7 +30,6 @@ export const enableAsyncIterableChildren = false; export const enableCPUSuspense = false; export const enableCreateEventHandleAPI = false; export const enableDO_NOT_USE_disableStrictPassiveEffect = false; -export const enableFabricCompleteRootInCommitPhase = false; export const enableMoveBefore = true; export const enableFizzExternalRuntime = true; export const enableHalt = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index e4295dec55..21a95d51cd 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -36,7 +36,6 @@ export const enableUseEffectEventHook = false; export const favorSafetyOverHydrationPerf = true; export const enableLegacyFBSupport = false; export const enableMoveBefore = false; -export const enableFabricCompleteRootInCommitPhase = false; export const enableHiddenSubtreeInsertionEffectCleanup = false; export const enableHydrationLaneScheduling = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js index dcded1c0a6..478e1d7953 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js @@ -60,7 +60,6 @@ export const renameElementSymbol = false; export const retryLaneExpirationMs = 5000; export const syncLaneExpirationMs = 250; export const transitionLaneExpirationMs = 5000; -export const enableFabricCompleteRootInCommitPhase = false; export const enableSiblingPrerendering = true; export const enableHydrationLaneScheduling = true; export const enableYieldingBeforePassive = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 71d6f09abb..247950f1fb 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -39,7 +39,6 @@ export const favorSafetyOverHydrationPerf = true; export const enableLegacyFBSupport = false; export const enableMoveBefore = false; export const enableRenderableContext = false; -export const enableFabricCompleteRootInCommitPhase = false; export const enableHiddenSubtreeInsertionEffectCleanup = true; export const enableRetryLaneExpiration = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 079b6e8e92..78ff747a06 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -48,7 +48,6 @@ export const enableProfilerTimer = __PROFILE__; export const enableProfilerCommitHooks = __PROFILE__; export const enableProfilerNestedUpdatePhase = __PROFILE__; export const enableUpdaterTracking = __PROFILE__; -export const enableFabricCompleteRootInCommitPhase = false; export const enableSuspenseAvoidThisFallback = true; From 79586c7eb626c6b9362c308a54c9ee5b66e640e5 Mon Sep 17 00:00:00 2001 From: Matt Carroll <7158882+mattcarrollcode@users.noreply.github.com> Date: Mon, 5 May 2025 14:47:47 -0700 Subject: [PATCH 08/31] Add test for multiple form submissions (#33059) Test for #30041 and #33055 --- .../src/__tests__/ReactDOMForm-test.js | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactDOMForm-test.js b/packages/react-dom/src/__tests__/ReactDOMForm-test.js index 93edccf9bc..f981ed4c38 100644 --- a/packages/react-dom/src/__tests__/ReactDOMForm-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMForm-test.js @@ -1670,6 +1670,37 @@ describe('ReactDOMForm', () => { expect(divRef.current.textContent).toEqual('Current username: acdlite'); }); + it('parallel form submissions do not throw', async () => { + const formRef = React.createRef(); + let resolve = null; + function App() { + async function submitForm() { + Scheduler.log('Action'); + if (!resolve) { + await new Promise(res => { + resolve = res; + }); + } + } + return
; + } + const root = ReactDOMClient.createRoot(container); + await act(() => root.render()); + + // Start first form submission + await act(async () => { + formRef.current.requestSubmit(); + }); + assertLog(['Action']); + + // Submit form again while first form action is still pending + await act(async () => { + formRef.current.requestSubmit(); + resolve(); // Resolve the promise to allow the first form action to complete + }); + assertLog(['Action']); + }); + it( 'requestFormReset works with inputs that are not descendants ' + 'of the form element', From 587cb8f8967866139bbfdbae3f519cb37e68a054 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 6 May 2025 00:10:05 -0400 Subject: [PATCH 09/31] [Fiber] Replay onChange Events if input/textarea/select has changed before hydration (#33129) This fixes a long standing issue that controlled inputs gets out of sync with the browser state if it's changed before we hydrate. This resolves the issue by replaying the change events (click, input and change) if the value has changed by the time we commit the hydration. That way you can reflect the new value in state to bring it in sync. It does this whether controlled or uncontrolled. The idea is that this should be ok to replay because it's similar to the continuous events in that it doesn't replay a sequence but only reflects the current state of the tree. Since this is a breaking change I added it behind `enableHydrationChangeEvent` flag. There is still an additional issue remaining that I intend to address in a follow up. If a `useLayoutEffect` triggers an sync rerender on hydration (always a bad idea) then that can rerender before we have had a chance to replay the change events. If that renders through a input then that input will always override the browser value with the controlled value. Which will reset it before we've had a change to update to the new value. --- .../src/client/ReactDOMComponent.js | 37 +++++----- .../src/client/ReactDOMInput.js | 51 +++++++++++-- .../src/client/ReactDOMSelect.js | 56 ++++++++++++++- .../src/client/ReactDOMTextarea.js | 30 ++++++++ .../src/client/ReactFiberConfigDOM.js | 72 +++++++++++++++++++ .../src/client/inputValueTracking.js | 46 +++++++++--- .../src/events/ReactDOMEventReplaying.js | 55 ++++++++++++-- .../src/__tests__/ReactDOMInput-test.js | 45 ++++++++---- ...OMServerIntegrationUserInteraction-test.js | 33 ++++----- .../src/ReactFiberCommitHostEffects.js | 23 ++++++ .../src/ReactFiberCommitWork.js | 10 ++- .../src/ReactFiberCompleteWork.js | 12 ++++ .../src/ReactFiberConfigWithNoHydration.js | 2 + .../react-reconciler/src/ReactFiberFlags.js | 1 + .../src/forks/ReactFiberConfig.custom.js | 2 + packages/shared/ReactFeatureFlags.js | 2 + .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + ...actFeatureFlags.test-renderer.native-fb.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 1 + 22 files changed, 419 insertions(+), 64 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js index 2343445ae0..8ae6021aec 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMComponent.js +++ b/packages/react-dom-bindings/src/client/ReactDOMComponent.js @@ -49,7 +49,6 @@ import { } from './ReactDOMTextarea'; import {setSrcObject} from './ReactDOMSrcObject'; import {validateTextNesting} from './validateDOMNesting'; -import {track} from './inputValueTracking'; import setTextContent from './setTextContent'; import { createDangerousStringForStyles, @@ -67,6 +66,7 @@ import sanitizeURL from '../shared/sanitizeURL'; import {trackHostMutation} from 'react-reconciler/src/ReactFiberMutationTracking'; import { + enableHydrationChangeEvent, enableScrollEndPolyfill, enableSrcObject, enableTrustedTypesIntegration, @@ -1187,7 +1187,6 @@ export function setInitialProperties( name, false, ); - track((domElement: any)); return; } case 'select': { @@ -1285,7 +1284,6 @@ export function setInitialProperties( // up necessary since we never stop tracking anymore. validateTextareaProps(domElement, props); initTextarea(domElement, value, defaultValue, children); - track((domElement: any)); return; } case 'option': { @@ -3100,17 +3098,18 @@ export function hydrateProperties( // option and select we don't quite do the same thing and select // is not resilient to the DOM state changing so we don't do that here. // TODO: Consider not doing this for input and textarea. - initInput( - domElement, - props.value, - props.defaultValue, - props.checked, - props.defaultChecked, - props.type, - props.name, - true, - ); - track((domElement: any)); + if (!enableHydrationChangeEvent) { + initInput( + domElement, + props.value, + props.defaultValue, + props.checked, + props.defaultChecked, + props.type, + props.name, + true, + ); + } break; case 'option': validateOptionProps(domElement, props); @@ -3134,8 +3133,14 @@ export function hydrateProperties( // TODO: Make sure we check if this is still unmounted or do any clean // up necessary since we never stop tracking anymore. validateTextareaProps(domElement, props); - initTextarea(domElement, props.value, props.defaultValue, props.children); - track((domElement: any)); + if (!enableHydrationChangeEvent) { + initTextarea( + domElement, + props.value, + props.defaultValue, + props.children, + ); + } break; } diff --git a/packages/react-dom-bindings/src/client/ReactDOMInput.js b/packages/react-dom-bindings/src/client/ReactDOMInput.js index 33c04e48d0..b6e665e128 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMInput.js +++ b/packages/react-dom-bindings/src/client/ReactDOMInput.js @@ -12,13 +12,17 @@ import {getCurrentFiberOwnerNameInDevOrNull} from 'react-reconciler/src/ReactCur import {getFiberCurrentPropsFromNode} from './ReactDOMComponentTree'; import {getToStringValue, toString} from './ToStringValue'; -import {updateValueIfChanged} from './inputValueTracking'; +import {track, trackHydrated, updateValueIfChanged} from './inputValueTracking'; import getActiveElement from './getActiveElement'; -import {disableInputAttributeSyncing} from 'shared/ReactFeatureFlags'; +import { + disableInputAttributeSyncing, + enableHydrationChangeEvent, +} from 'shared/ReactFeatureFlags'; import {checkAttributeStringCoercion} from 'shared/CheckStringCoercion'; import type {ToStringValue} from './ToStringValue'; import escapeSelectorAttributeValueInsideDoubleQuotes from './escapeSelectorAttributeValueInsideDoubleQuotes'; +import {queueChangeEvent} from '../events/ReactDOMEventReplaying'; let didWarnValueDefaultValue = false; let didWarnCheckedDefaultChecked = false; @@ -229,6 +233,8 @@ export function initInput( // Avoid setting value attribute on submit/reset inputs as it overrides the // default value provided by the browser. See: #12872 if (isButton && (value === undefined || value === null)) { + // We track the value just in case it changes type later on. + track((element: any)); return; } @@ -239,7 +245,7 @@ export function initInput( // Do not assign value if it is already set. This prevents user text input // from being lost during SSR hydration. - if (!isHydrating) { + if (!isHydrating || enableHydrationChangeEvent) { if (disableInputAttributeSyncing) { // When not syncing the value attribute, the value property points // directly to the React prop. Only assign it if it exists. @@ -297,7 +303,7 @@ export function initInput( typeof checkedOrDefault !== 'symbol' && !!checkedOrDefault; - if (isHydrating) { + if (isHydrating && !enableHydrationChangeEvent) { // Detach .checked from .defaultChecked but leave user input alone node.checked = node.checked; } else { @@ -335,6 +341,43 @@ export function initInput( } node.name = name; } + track((element: any)); +} + +export function hydrateInput( + element: Element, + value: ?string, + defaultValue: ?string, + checked: ?boolean, + defaultChecked: ?boolean, +): void { + const node: HTMLInputElement = (element: any); + + const defaultValueStr = + defaultValue != null ? toString(getToStringValue(defaultValue)) : ''; + const initialValue = + value != null ? toString(getToStringValue(value)) : defaultValueStr; + + const checkedOrDefault = checked != null ? checked : defaultChecked; + // TODO: This 'function' or 'symbol' check isn't replicated in other places + // so this semantic is inconsistent. + const initialChecked = + typeof checkedOrDefault !== 'function' && + typeof checkedOrDefault !== 'symbol' && + !!checkedOrDefault; + + // Detach .checked from .defaultChecked but leave user input alone + node.checked = node.checked; + + const changed = trackHydrated((node: any), initialValue, initialChecked); + if (changed) { + // If the current value is different, that suggests that the user + // changed it before hydration. Queue a replay of the change event. + // For radio buttons the change event only fires on the selected one. + if (node.type !== 'radio' || node.checked) { + queueChangeEvent(node); + } + } } export function restoreControlledInputState(element: Element, props: Object) { diff --git a/packages/react-dom-bindings/src/client/ReactDOMSelect.js b/packages/react-dom-bindings/src/client/ReactDOMSelect.js index 984abbc07c..00136aa817 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMSelect.js +++ b/packages/react-dom-bindings/src/client/ReactDOMSelect.js @@ -12,6 +12,7 @@ import {getCurrentFiberOwnerNameInDevOrNull} from 'react-reconciler/src/ReactCur import {getToStringValue, toString} from './ToStringValue'; import isArray from 'shared/isArray'; +import {queueChangeEvent} from '../events/ReactDOMEventReplaying'; let didWarnValueDefaultValue; @@ -86,7 +87,7 @@ function updateOptions( } else { // Do not set `select.value` as exact behavior isn't consistent across all // browsers for all cases. - const selectedValue = toString(getToStringValue((propValue: any))); + const selectedValue = toString(getToStringValue(propValue)); let defaultSelected = null; for (let i = 0; i < options.length; i++) { if (options[i].value === selectedValue) { @@ -157,6 +158,59 @@ export function initSelect( } } +export function hydrateSelect( + element: Element, + value: ?string, + defaultValue: ?string, + multiple: ?boolean, +): void { + const node: HTMLSelectElement = (element: any); + const options: HTMLOptionsCollection = node.options; + + const propValue: any = value != null ? value : defaultValue; + + let changed = false; + + if (multiple) { + const selectedValues = (propValue: ?Array); + const selectedValue: {[string]: boolean} = {}; + if (selectedValues != null) { + for (let i = 0; i < selectedValues.length; i++) { + // Prefix to avoid chaos with special keys. + selectedValue['$' + selectedValues[i]] = true; + } + } + for (let i = 0; i < options.length; i++) { + const expectedSelected = selectedValue.hasOwnProperty( + '$' + options[i].value, + ); + if (options[i].selected !== expectedSelected) { + changed = true; + break; + } + } + } else { + let selectedValue = + propValue == null ? null : toString(getToStringValue(propValue)); + for (let i = 0; i < options.length; i++) { + if (selectedValue == null && !options[i].disabled) { + // We expect the first non-disabled option to be selected if the selected is null. + selectedValue = options[i].value; + } + const expectedSelected = options[i].value === selectedValue; + if (options[i].selected !== expectedSelected) { + changed = true; + break; + } + } + } + if (changed) { + // If the current selection is different than our initial that suggests that the user + // changed it before hydration. Queue a replay of the change event. + queueChangeEvent(node); + } +} + export function updateSelect( element: Element, value: ?string, diff --git a/packages/react-dom-bindings/src/client/ReactDOMTextarea.js b/packages/react-dom-bindings/src/client/ReactDOMTextarea.js index b0a1f520fd..bc346b4bce 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMTextarea.js +++ b/packages/react-dom-bindings/src/client/ReactDOMTextarea.js @@ -13,6 +13,9 @@ import {getCurrentFiberOwnerNameInDevOrNull} from 'react-reconciler/src/ReactCur import {getToStringValue, toString} from './ToStringValue'; import {disableTextareaChildren} from 'shared/ReactFeatureFlags'; +import {track, trackHydrated} from './inputValueTracking'; +import {queueChangeEvent} from '../events/ReactDOMEventReplaying'; + let didWarnValDefaultVal = false; /** @@ -140,6 +143,33 @@ export function initTextarea( node.value = textContent; } } + + track((element: any)); +} + +export function hydrateTextarea( + element: Element, + value: ?string, + defaultValue: ?string, +): void { + const node: HTMLTextAreaElement = (element: any); + let initialValue = value; + if (initialValue == null) { + if (defaultValue == null) { + defaultValue = ''; + } + initialValue = defaultValue; + } + // Track the value that we last observed which is the hydrated value so + // that any change event that fires will trigger onChange on the actual + // current value. + const stringValue = toString(getToStringValue(initialValue)); + const changed = trackHydrated((node: any), stringValue, false); + if (changed) { + // If the current value is different, that suggests that the user + // changed it before hydration. Queue a replay of the change event. + queueChangeEvent(node); + } } export function restoreControlledTextareaState( diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index e1cd27b69b..1f7a4f915c 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -75,6 +75,9 @@ import { diffHydratedText, trapClickOnNonInteractiveElement, } from './ReactDOMComponent'; +import {hydrateInput} from './ReactDOMInput'; +import {hydrateTextarea} from './ReactDOMTextarea'; +import {hydrateSelect} from './ReactDOMSelect'; import {getSelectionInformation, restoreSelection} from './ReactInputSelection'; import setTextContent from './setTextContent'; import { @@ -108,6 +111,7 @@ import { enableSuspenseyImages, enableSrcObject, enableViewTransition, + enableHydrationChangeEvent, } from 'shared/ReactFeatureFlags'; import { HostComponent, @@ -154,6 +158,10 @@ export type Props = { top?: null | number, is?: string, size?: number, + value?: string, + defaultValue?: string, + checked?: boolean, + defaultChecked?: boolean, multiple?: boolean, src?: string | Blob | MediaSource | MediaStream, // TODO: Response srcSet?: string, @@ -611,6 +619,27 @@ export function finalizeInitialChildren( } } +export function finalizeHydratedChildren( + domElement: Instance, + type: string, + props: Props, + hostContext: HostContext, +): boolean { + // TOOD: Consider unifying this with hydrateInstance. + if (!enableHydrationChangeEvent) { + return false; + } + switch (type) { + case 'input': + case 'select': + case 'textarea': + case 'img': + return true; + default: + return false; + } +} + export function shouldSetTextContent(type: string, props: Props): boolean { return ( type === 'textarea' || @@ -819,6 +848,49 @@ export function commitMount( } } +export function commitHydratedInstance( + domElement: Instance, + type: string, + props: Props, + internalInstanceHandle: Object, +): void { + if (!enableHydrationChangeEvent) { + return; + } + // This fires in the commit phase if a hydrated instance needs to do further + // work in the commit phase. Similar to commitMount. However, this should not + // do things that would've already happened such as set auto focus since that + // would steal focus. It's only scheduled if finalizeHydratedChildren returns + // true. + switch (type) { + case 'input': { + hydrateInput( + domElement, + props.value, + props.defaultValue, + props.checked, + props.defaultChecked, + ); + break; + } + case 'select': { + hydrateSelect( + domElement, + props.value, + props.defaultValue, + props.multiple, + ); + break; + } + case 'textarea': + hydrateTextarea(domElement, props.value, props.defaultValue); + break; + case 'img': + // TODO: Should we replay onLoad events? + break; + } +} + export function commitUpdate( domElement: Instance, type: string, diff --git a/packages/react-dom-bindings/src/client/inputValueTracking.js b/packages/react-dom-bindings/src/client/inputValueTracking.js index 5e1ca58687..f89617fe6d 100644 --- a/packages/react-dom-bindings/src/client/inputValueTracking.js +++ b/packages/react-dom-bindings/src/client/inputValueTracking.js @@ -51,18 +51,16 @@ function getValueFromNode(node: HTMLInputElement): string { return value; } -function trackValueOnNode(node: any): ?ValueTracker { - const valueField = isCheckable(node) ? 'checked' : 'value'; +function trackValueOnNode( + node: any, + valueField: 'checked' | 'value', + currentValue: string, +): ?ValueTracker { const descriptor = Object.getOwnPropertyDescriptor( node.constructor.prototype, valueField, ); - if (__DEV__) { - checkFormFieldValueStringCoercion(node[valueField]); - } - let currentValue = '' + node[valueField]; - // if someone has already defined a value or Safari, then bail // and don't track value will cause over reporting of changes, // but it's better then a hard failure @@ -123,7 +121,39 @@ export function track(node: ElementWithValueTracker) { return; } - node._valueTracker = trackValueOnNode(node); + const valueField = isCheckable(node) ? 'checked' : 'value'; + // This is read from the DOM so always safe to coerce. We really shouldn't + // be coercing to a string at all. It's just historical. + // eslint-disable-next-line react-internal/safe-string-coercion + const initialValue = '' + (node[valueField]: any); + node._valueTracker = trackValueOnNode(node, valueField, initialValue); +} + +export function trackHydrated( + node: ElementWithValueTracker, + initialValue: string, + initialChecked: boolean, +): boolean { + // For hydration, the initial value is not the current value but the value + // that we last observed which is what the initial server render was. + if (getTracker(node)) { + return false; + } + + let valueField; + let expectedValue; + if (isCheckable(node)) { + valueField = 'checked'; + // eslint-disable-next-line react-internal/safe-string-coercion + expectedValue = '' + (initialChecked: any); + } else { + valueField = 'value'; + expectedValue = initialValue; + } + // eslint-disable-next-line react-internal/safe-string-coercion + const currentValue = '' + (node[valueField]: any); + node._valueTracker = trackValueOnNode(node, valueField, expectedValue); + return currentValue !== expectedValue; } export function updateValueIfChanged(node: ElementWithValueTracker): boolean { diff --git a/packages/react-dom-bindings/src/events/ReactDOMEventReplaying.js b/packages/react-dom-bindings/src/events/ReactDOMEventReplaying.js index a6f6f3055a..d600917534 100644 --- a/packages/react-dom-bindings/src/events/ReactDOMEventReplaying.js +++ b/packages/react-dom-bindings/src/events/ReactDOMEventReplaying.js @@ -56,9 +56,11 @@ import { attemptHydrationAtCurrentPriority, } from 'react-reconciler/src/ReactFiberReconciler'; +import {enableHydrationChangeEvent} from 'shared/ReactFeatureFlags'; + // TODO: Upgrade this definition once we're on a newer version of Flow that // has this definition built-in. -type PointerEvent = Event & { +type PointerEventType = Event & { pointerId: number, relatedTarget: EventTarget | null, ... @@ -84,6 +86,8 @@ const queuedPointers: Map = new Map(); const queuedPointerCaptures: Map = new Map(); // We could consider replaying selectionchange and touchmoves too. +const queuedChangeEventTargets: Array = []; + type QueuedHydrationTarget = { blockedOn: null | Container | ActivityInstance | SuspenseInstance, target: Node, @@ -164,13 +168,13 @@ export function clearIfContinuousEvent( break; case 'pointerover': case 'pointerout': { - const pointerId = ((nativeEvent: any): PointerEvent).pointerId; + const pointerId = ((nativeEvent: any): PointerEventType).pointerId; queuedPointers.delete(pointerId); break; } case 'gotpointercapture': case 'lostpointercapture': { - const pointerId = ((nativeEvent: any): PointerEvent).pointerId; + const pointerId = ((nativeEvent: any): PointerEventType).pointerId; queuedPointerCaptures.delete(pointerId); break; } @@ -268,7 +272,7 @@ export function queueIfContinuousEvent( return true; } case 'pointerover': { - const pointerEvent = ((nativeEvent: any): PointerEvent); + const pointerEvent = ((nativeEvent: any): PointerEventType); const pointerId = pointerEvent.pointerId; queuedPointers.set( pointerId, @@ -284,7 +288,7 @@ export function queueIfContinuousEvent( return true; } case 'gotpointercapture': { - const pointerEvent = ((nativeEvent: any): PointerEvent); + const pointerEvent = ((nativeEvent: any): PointerEventType); const pointerId = pointerEvent.pointerId; queuedPointerCaptures.set( pointerId, @@ -421,6 +425,31 @@ function attemptReplayContinuousQueuedEventInMap( } } +function replayChangeEvent(target: EventTarget): void { + // Dispatch a fake "change" event for the input. + const element: HTMLInputElement | HTMLTextAreaElement | HTMLSelectElement = + (target: any); + if (element.nodeName === 'INPUT') { + if (element.type === 'checkbox' || element.type === 'radio') { + // Checkboxes always fire a click event regardless of how the change was made. + const EventCtr = + typeof PointerEvent === 'function' ? PointerEvent : Event; + target.dispatchEvent(new EventCtr('click', {bubbles: true})); + // For checkboxes the input event uses the Event constructor instead of InputEvent. + target.dispatchEvent(new Event('input', {bubbles: true})); + } else { + if (typeof InputEvent === 'function') { + target.dispatchEvent(new InputEvent('input', {bubbles: true})); + } + } + } else if (element.nodeName === 'TEXTAREA') { + if (typeof InputEvent === 'function') { + target.dispatchEvent(new InputEvent('input', {bubbles: true})); + } + } + target.dispatchEvent(new Event('change', {bubbles: true})); +} + function replayUnblockedEvents() { hasScheduledReplayAttempt = false; // Replay any continuous events. @@ -435,6 +464,22 @@ function replayUnblockedEvents() { } queuedPointers.forEach(attemptReplayContinuousQueuedEventInMap); queuedPointerCaptures.forEach(attemptReplayContinuousQueuedEventInMap); + if (enableHydrationChangeEvent) { + for (let i = 0; i < queuedChangeEventTargets.length; i++) { + replayChangeEvent(queuedChangeEventTargets[i]); + } + queuedChangeEventTargets.length = 0; + } +} + +export function queueChangeEvent(target: EventTarget): void { + if (enableHydrationChangeEvent) { + queuedChangeEventTargets.push(target); + if (!hasScheduledReplayAttempt) { + hasScheduledReplayAttempt = true; + scheduleCallback(NormalPriority, replayUnblockedEvents); + } + } } function scheduleCallbackIfUnblocked( diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index 5b47095d7c..04bd96fe2e 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -1536,10 +1536,14 @@ describe('ReactDOMInput', () => { ReactDOMClient.hydrateRoot(container, ); }); - // Currently, we don't fire onChange when hydrating - assertLog([]); - // Strangely, we leave `b` checked even though we rendered A with - // checked={true} and B with checked={false}. Arguably this is a bug. + if (gate(flags => flags.enableHydrationChangeEvent)) { + // We replayed the click since the value changed before hydration. + assertLog(['click b']); + } else { + assertLog([]); + // Strangely, we leave `b` checked even though we rendered A with + // checked={true} and B with checked={false}. Arguably this is a bug. + } expect(a.checked).toBe(false); expect(b.checked).toBe(true); expect(c.checked).toBe(false); @@ -1554,22 +1558,35 @@ describe('ReactDOMInput', () => { dispatchEventOnNode(c, 'click'); }); - // then since C's onClick doesn't set state, A becomes rechecked. assertLog(['click c']); - expect(a.checked).toBe(true); - expect(b.checked).toBe(false); - expect(c.checked).toBe(false); + if (gate(flags => flags.enableHydrationChangeEvent)) { + // then since C's onClick doesn't set state, B becomes rechecked. + expect(a.checked).toBe(false); + expect(b.checked).toBe(true); + expect(c.checked).toBe(false); + } else { + // then since C's onClick doesn't set state, A becomes rechecked + // since in this branch we didn't replay to select B. + expect(a.checked).toBe(true); + expect(b.checked).toBe(false); + expect(c.checked).toBe(false); + } expect(isCheckedDirty(a)).toBe(true); expect(isCheckedDirty(b)).toBe(true); expect(isCheckedDirty(c)).toBe(true); assertInputTrackingIsCurrent(container); - // And we can also change to B properly after hydration. await act(async () => { setUntrackedChecked.call(b, true); dispatchEventOnNode(b, 'click'); }); - assertLog(['click b']); + if (gate(flags => flags.enableHydrationChangeEvent)) { + // Since we already had this selected, this doesn't trigger a change again. + assertLog([]); + } else { + // And we can also change to B properly after hydration. + assertLog(['click b']); + } expect(a.checked).toBe(false); expect(b.checked).toBe(true); expect(c.checked).toBe(false); @@ -1628,8 +1645,12 @@ describe('ReactDOMInput', () => { ReactDOMClient.hydrateRoot(container, ); }); - // Currently, we don't fire onChange when hydrating - assertLog([]); + if (gate(flags => flags.enableHydrationChangeEvent)) { + // We replayed the click since the value changed before hydration. + assertLog(['click b']); + } else { + assertLog([]); + } expect(a.checked).toBe(false); expect(b.checked).toBe(true); expect(c.checked).toBe(false); diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationUserInteraction-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationUserInteraction-test.js index 1cae7f15b0..be0d4533af 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationUserInteraction-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationUserInteraction-test.js @@ -278,10 +278,9 @@ describe('ReactDOMServerIntegrationUserInteraction', () => { await testUserInteractionBeforeClientRender( changeCount++} />, ); - // note that there's a strong argument to be made that the DOM revival - // algorithm should notice that the user has changed the value and fire - // an onChange. however, it does not now, so that's what this tests. - expect(changeCount).toBe(0); + expect(changeCount).toBe( + gate(flags => flags.enableHydrationChangeEvent) ? 1 : 0, + ); }); it('should not blow away user-interaction on successful reconnect to an uncontrolled range input', () => @@ -302,7 +301,9 @@ describe('ReactDOMServerIntegrationUserInteraction', () => { '0.25', '1', ); - expect(changeCount).toBe(0); + expect(changeCount).toBe( + gate(flags => flags.enableHydrationChangeEvent) ? 1 : 0, + ); }); it('should not blow away user-entered text on successful reconnect to an uncontrolled checkbox', () => @@ -321,24 +322,22 @@ describe('ReactDOMServerIntegrationUserInteraction', () => { false, 'checked', ); - expect(changeCount).toBe(0); + expect(changeCount).toBe( + gate(flags => flags.enableHydrationChangeEvent) ? 1 : 0, + ); }); - // skipping this test because React 15 does the wrong thing. it blows - // away the user's typing in the textarea. - // eslint-disable-next-line jest/no-disabled-tests - it.skip('should not blow away user-entered text on successful reconnect to an uncontrolled textarea', () => + // @gate enableHydrationChangeEvent + it('should not blow away user-entered text on successful reconnect to an uncontrolled textarea', () => testUserInteractionBeforeClientRender(