From 77c4ac2ce88736bbdfe0b29008b5df931c2beb1e Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 31 Oct 2023 23:32:31 -0400 Subject: [PATCH] [useFormState] Allow sync actions (#27571) Updates useFormState to allow a sync function to be passed as an action. A form action is almost always async, because it needs to talk to the server. But since we support client-side actions, too, there's no reason we can't allow sync actions, too. I originally chose not to allow them to keep the implementation simpler but it's not really that much more complicated because we already support this for actions passed to startTransition. So now it's consistent: anywhere an action is accepted, a sync client function is a valid input. --- fixtures/flight/config/modules.js | 8 +- package.json | 4 +- .../react-client/src/ReactFlightClient.js | 10 +- .../profilerChangeDescriptions-test.js | 11 +- .../src/devtools/ProfilingCache.js | 16 +- .../hooks/__tests__/parseHookNames-test.js | 5 +- .../server/ReactDOMServerExternalRuntime.js | 16 +- .../src/shared/ReactDOMFormActions.js | 7 +- packages/react-dom/index.experimental.js | 7 +- packages/react-dom/server-rendering-stub.js | 7 +- .../src/__tests__/ReactDOMForm-test.js | 184 +++++++++++++++-- .../src/__tests__/ReactDOMInput-test.js | 193 +++++++++--------- .../src/__tests__/ReactDOMTextarea-test.js | 36 ++-- .../InitializeNativeFabricUIManager.js | 119 +++++------ .../Libraries/ReactPrivate/UIManager.js | 29 ++- .../__tests__/ReactFabric-test.internal.js | 15 +- .../src/ReactFiberAsyncAction.js | 4 +- .../react-reconciler/src/ReactFiberHooks.js | 191 +++++++++-------- .../src/ReactInternalTypes.js | 7 +- .../src/__tests__/ReactNewContext-test.js | 19 +- .../src/__tests__/ReactFlightDOMForm-test.js | 72 +++---- packages/react-server/src/ReactFizzHooks.js | 7 +- .../react/src/__tests__/forwardRef-test.js | 11 +- packages/shared/ReactTypes.js | 10 + scripts/circleci/run_devtools_e2e_tests.js | 7 +- scripts/error-codes/invertObject.js | 2 +- scripts/prettier/index.js | 86 ++++---- scripts/print-warnings/print-warnings.js | 5 + .../rollup/generate-inline-fizz-runtime.js | 2 +- scripts/shared/evalToString.js | 9 +- yarn.lock | 41 +++- 31 files changed, 668 insertions(+), 472 deletions(-) diff --git a/fixtures/flight/config/modules.js b/fixtures/flight/config/modules.js index 79723ef3f5..91f8abd0cb 100644 --- a/fixtures/flight/config/modules.js +++ b/fixtures/flight/config/modules.js @@ -108,9 +108,11 @@ function getModules() { // TypeScript project and set up the config // based on tsconfig.json if (hasTsConfig) { - const ts = require(resolve.sync('typescript', { - basedir: paths.appNodeModules, - })); + const ts = require( + resolve.sync('typescript', { + basedir: paths.appNodeModules, + }) + ); config = ts.readConfigFile(paths.appTsConfig, ts.sys.readFile).config; // Otherwise we'll check if there is jsconfig.json // for non TS projects. diff --git a/package.json b/package.json index d45f2f57c4..879846274c 100644 --- a/package.json +++ b/package.json @@ -81,14 +81,14 @@ "minimist": "^1.2.3", "mkdirp": "^0.5.1", "ncp": "^2.0.0", - "prettier": "2.8.3", + "prettier": "3.0.3", "pretty-format": "^29.4.1", "prop-types": "^15.6.2", "random-seed": "^0.3.0", "react-lifecycles-compat": "^3.0.4", "rimraf": "^3.0.0", "rollup": "^3.17.1", - "rollup-plugin-prettier": "^3.0.0", + "rollup-plugin-prettier": "^4.1.1", "rollup-plugin-strip-banner": "^3.0.0", "semver": "^7.1.1", "signedsource": "^2.0.0", diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 8c584bbc59..a62fb685fb 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -572,11 +572,11 @@ function createServerReferenceProxy, T>( } // Since this is a fake Promise whose .then doesn't chain, we have to wrap it. // TODO: Remove the wrapper once that's fixed. - return ((Promise.resolve(p): any): Promise>).then(function ( - bound, - ) { - return callServer(metaData.id, bound.concat(args)); - }); + return ((Promise.resolve(p): any): Promise>).then( + function (bound) { + return callServer(metaData.id, bound.concat(args)); + }, + ); }; registerServerReference(proxy, metaData); return proxy; diff --git a/packages/react-devtools-shared/src/__tests__/profilerChangeDescriptions-test.js b/packages/react-devtools-shared/src/__tests__/profilerChangeDescriptions-test.js index b93aa49163..3536034d9f 100644 --- a/packages/react-devtools-shared/src/__tests__/profilerChangeDescriptions-test.js +++ b/packages/react-devtools-shared/src/__tests__/profilerChangeDescriptions-test.js @@ -40,12 +40,11 @@ describe('Profiler change descriptions', () => { } const MemoizedChild = React.memo(Child, areEqual); - const ForwardRefChild = React.forwardRef(function RefForwardingComponent( - props, - ref, - ) { - return ; - }); + const ForwardRefChild = React.forwardRef( + function RefForwardingComponent(props, ref) { + return ; + }, + ); let forceUpdate = null; diff --git a/packages/react-devtools-shared/src/devtools/ProfilingCache.js b/packages/react-devtools-shared/src/devtools/ProfilingCache.js index 4a6dcc50c2..e0c86b2aba 100644 --- a/packages/react-devtools-shared/src/devtools/ProfilingCache.js +++ b/packages/react-devtools-shared/src/devtools/ProfilingCache.js @@ -33,20 +33,20 @@ export default class ProfilingCache { this._profilerStore = profilerStore; } - getCommitTree: ({ - commitIndex: number, - rootID: number, - }) => CommitTree = ({commitIndex, rootID}) => + getCommitTree: ({commitIndex: number, rootID: number}) => CommitTree = ({ + commitIndex, + rootID, + }) => getCommitTree({ commitIndex, profilerStore: this._profilerStore, rootID, }); - getFiberCommits: ({ - fiberID: number, - rootID: number, - }) => Array = ({fiberID, rootID}) => { + getFiberCommits: ({fiberID: number, rootID: number}) => Array = ({ + fiberID, + rootID, + }) => { const cachedFiberCommits = this._fiberCommits.get(fiberID); if (cachedFiberCommits != null) { return cachedFiberCommits; diff --git a/packages/react-devtools-shared/src/hooks/__tests__/parseHookNames-test.js b/packages/react-devtools-shared/src/hooks/__tests__/parseHookNames-test.js index ccf6104bec..01135909b5 100644 --- a/packages/react-devtools-shared/src/hooks/__tests__/parseHookNames-test.js +++ b/packages/react-devtools-shared/src/hooks/__tests__/parseHookNames-test.js @@ -70,9 +70,8 @@ describe('parseHookNames', () => { const hooksList = flattenHooksList(hooksTree); // Runs in the UI thread so it can share Network cache: - const locationKeyToHookSourceAndMetadata = await loadSourceAndMetadata( - hooksList, - ); + const locationKeyToHookSourceAndMetadata = + await loadSourceAndMetadata(hooksList); // Runs in a Worker because it's CPU intensive: return parseSourceAndMetadata( diff --git a/packages/react-dom-bindings/src/server/ReactDOMServerExternalRuntime.js b/packages/react-dom-bindings/src/server/ReactDOMServerExternalRuntime.js index bd6ce50116..da00da8e66 100644 --- a/packages/react-dom-bindings/src/server/ReactDOMServerExternalRuntime.js +++ b/packages/react-dom-bindings/src/server/ReactDOMServerExternalRuntime.js @@ -26,7 +26,7 @@ if (document.body != null) { installFizzInstrObserver(document.body); } // $FlowFixMe[incompatible-cast] - handleExistingNodes((document.body /*: HTMLElement */)); + handleExistingNodes((document.body: HTMLElement)); } else { // Document must be loading -- body may not exist yet if the fizz external // runtime is sent in (e.g. as a preinit resource) @@ -38,7 +38,7 @@ if (document.body != null) { installFizzInstrObserver(document.body); } // $FlowFixMe[incompatible-cast] - handleExistingNodes((document.body /*: HTMLElement */)); + handleExistingNodes((document.body: HTMLElement)); // We can call disconnect without takeRecord here, // since we only expect a single document.body @@ -49,15 +49,15 @@ if (document.body != null) { domBodyObserver.observe(document.documentElement, {childList: true}); } -function handleExistingNodes(target /*: HTMLElement */) { +function handleExistingNodes(target: HTMLElement) { const existingNodes = target.querySelectorAll('template'); for (let i = 0; i < existingNodes.length; i++) { handleNode(existingNodes[i]); } } -function installFizzInstrObserver(target /*: Node */) { - const handleMutations = (mutations /*: Array */) => { +function installFizzInstrObserver(target: Node) { + const handleMutations = (mutations: Array) => { for (let i = 0; i < mutations.length; i++) { const addedNodes = mutations[i].addedNodes; for (let j = 0; j < addedNodes.length; j++) { @@ -80,13 +80,13 @@ function installFizzInstrObserver(target /*: Node */) { }); } -function handleNode(node_ /*: Node */) { +function handleNode(node_: Node) { // $FlowFixMe[incompatible-cast] - if (node_.nodeType !== 1 || !(node_ /*: HTMLElement */).dataset) { + if (node_.nodeType !== 1 || !(node_: HTMLElement).dataset) { return; } // $FlowFixMe[incompatible-cast] - const node = (node_ /*: HTMLElement */); + const node = (node_: HTMLElement); const dataset = node.dataset; if (dataset['rxi'] != null) { clientRenderBoundary( diff --git a/packages/react-dom-bindings/src/shared/ReactDOMFormActions.js b/packages/react-dom-bindings/src/shared/ReactDOMFormActions.js index 8c0f764d6c..b2ac098bce 100644 --- a/packages/react-dom-bindings/src/shared/ReactDOMFormActions.js +++ b/packages/react-dom-bindings/src/shared/ReactDOMFormActions.js @@ -8,6 +8,7 @@ */ import type {Dispatcher} from 'react-reconciler/src/ReactInternalTypes'; +import type {Awaited} from 'shared/ReactTypes'; import {enableAsyncActions, enableFormActions} from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; @@ -76,10 +77,10 @@ export function useFormStatus(): FormStatus { } export function useFormState( - action: (S, P) => Promise, - initialState: S, + action: (Awaited, P) => S, + initialState: Awaited, permalink?: string, -): [S, (P) => void] { +): [Awaited, (P) => void] { if (!(enableFormActions && enableAsyncActions)) { throw new Error('Not implemented.'); } else { diff --git a/packages/react-dom/index.experimental.js b/packages/react-dom/index.experimental.js index e946fee656..012eb6866e 100644 --- a/packages/react-dom/index.experimental.js +++ b/packages/react-dom/index.experimental.js @@ -31,6 +31,7 @@ export { version, } from './src/client/ReactDOM'; +import type {Awaited} from 'shared/ReactTypes'; import type {FormStatus} from 'react-dom-bindings/src/shared/ReactDOMFormActions'; import {useFormStatus, useFormState} from './src/client/ReactDOM'; @@ -45,10 +46,10 @@ export function experimental_useFormStatus(): FormStatus { } export function experimental_useFormState( - action: (S, P) => Promise, - initialState: S, + action: (Awaited, P) => S, + initialState: Awaited, permalink?: string, -): [S, (P) => void] { +): [Awaited, (P) => void] { if (__DEV__) { console.error( 'useFormState is now in canary. Remove the experimental_ prefix. ' + diff --git a/packages/react-dom/server-rendering-stub.js b/packages/react-dom/server-rendering-stub.js index d1c4c84518..71d4124dda 100644 --- a/packages/react-dom/server-rendering-stub.js +++ b/packages/react-dom/server-rendering-stub.js @@ -34,6 +34,7 @@ import { useFormStatus, useFormState, } from './src/server/ReactDOMServerRenderingStub'; +import type {Awaited} from 'shared/ReactTypes'; export function experimental_useFormStatus(): FormStatus { if (__DEV__) { @@ -46,10 +47,10 @@ export function experimental_useFormStatus(): FormStatus { } export function experimental_useFormState( - action: (S, P) => Promise, - initialState: S, + action: (Awaited, P) => S, + initialState: Awaited, permalink?: string, -): [S, (P) => void] { +): [Awaited, (P) => void] { if (__DEV__) { console.error( 'useFormState is now in canary. Remove the experimental_ prefix. ' + diff --git a/packages/react-dom/src/__tests__/ReactDOMForm-test.js b/packages/react-dom/src/__tests__/ReactDOMForm-test.js index 12c224949b..795909b6ea 100644 --- a/packages/react-dom/src/__tests__/ReactDOMForm-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMForm-test.js @@ -1113,29 +1113,179 @@ describe('ReactDOMForm', () => { // @gate enableFormActions // @gate enableAsyncActions - test('useFormState: warns if action is not async', async () => { - let dispatch; + test('useFormState: works if action is sync', async () => { + let increment; + function App({stepSize}) { + const [state, dispatch] = useFormState(prevState => { + return prevState + stepSize; + }, 0); + increment = dispatch; + return ; + } + + // Initial render + const root = ReactDOMClient.createRoot(container); + await act(() => root.render()); + assertLog([0]); + + // Perform an action. This will increase the state by 1, as defined by the + // stepSize prop. + await act(() => increment()); + assertLog([1]); + + // Now increase the stepSize prop to 10. Subsequent steps will increase + // by this amount. + await act(() => root.render()); + assertLog([1]); + + // Increment again. The state should increase by 10. + await act(() => increment()); + assertLog([11]); + }); + + // @gate enableFormActions + // @gate enableAsyncActions + test('useFormState: can mix sync and async actions', async () => { + let action; function App() { - const [state, _dispatch] = useFormState(() => {}, 0); - dispatch = _dispatch; + const [state, dispatch] = useFormState((s, a) => a, 'A'); + action = dispatch; return ; } const root = ReactDOMClient.createRoot(container); - await act(async () => { - root.render(); - }); - assertLog([0]); + await act(() => root.render()); + assertLog(['A']); - expect(() => { - // This throws because React expects the action to return a promise. - expect(() => dispatch()).toThrow('Cannot read properties of undefined'); - }).toErrorDev( - [ - // In dev we also log a warning. - 'The action passed to useFormState must be an async function', - ], - {withoutStack: true}, + await act(() => action(getText('B'))); + await act(() => action('C')); + await act(() => action(getText('D'))); + await act(() => action('E')); + + await act(() => resolveText('B')); + await act(() => resolveText('D')); + assertLog(['E']); + expect(container.textContent).toBe('E'); + }); + + // @gate enableFormActions + // @gate enableAsyncActions + test('useFormState: error handling (sync action)', async () => { + let resetErrorBoundary; + class ErrorBoundary extends React.Component { + state = {error: null}; + static getDerivedStateFromError(error) { + return {error}; + } + render() { + resetErrorBoundary = () => this.setState({error: null}); + if (this.state.error !== null) { + return ; + } + return this.props.children; + } + } + + let action; + function App() { + const [state, dispatch] = useFormState((s, a) => { + if (a.endsWith('!')) { + throw new Error(a); + } + return a; + }, 'A'); + action = dispatch; + return ; + } + + const root = ReactDOMClient.createRoot(container); + await act(() => + root.render( + + + , + ), ); + assertLog(['A']); + + await act(() => action('Oops!')); + assertLog(['Caught an error: Oops!', 'Caught an error: Oops!']); + expect(container.textContent).toBe('Caught an error: Oops!'); + + // Reset the error boundary + await act(() => resetErrorBoundary()); + assertLog(['A']); + + // Trigger an error again, but this time, perform another action that + // overrides the first one and fixes the error + await act(() => { + action('Oops!'); + action('B'); + }); + assertLog(['B']); + expect(container.textContent).toBe('B'); + }); + + // @gate enableFormActions + // @gate enableAsyncActions + test('useFormState: error handling (async action)', async () => { + let resetErrorBoundary; + class ErrorBoundary extends React.Component { + state = {error: null}; + static getDerivedStateFromError(error) { + return {error}; + } + render() { + resetErrorBoundary = () => this.setState({error: null}); + if (this.state.error !== null) { + return ; + } + return this.props.children; + } + } + + let action; + function App() { + const [state, dispatch] = useFormState(async (s, a) => { + const text = await getText(a); + if (text.endsWith('!')) { + throw new Error(text); + } + return text; + }, 'A'); + action = dispatch; + return ; + } + + const root = ReactDOMClient.createRoot(container); + await act(() => + root.render( + + + , + ), + ); + assertLog(['A']); + + await act(() => action('Oops!')); + assertLog([]); + await act(() => resolveText('Oops!')); + assertLog(['Caught an error: Oops!', 'Caught an error: Oops!']); + expect(container.textContent).toBe('Caught an error: Oops!'); + + // Reset the error boundary + await act(() => resetErrorBoundary()); + assertLog(['A']); + + // Trigger an error again, but this time, perform another action that + // overrides the first one and fixes the error + await act(() => { + action('Oops!'); + action('B'); + }); + assertLog([]); + await act(() => resolveText('B')); + assertLog(['B']); + expect(container.textContent).toBe('B'); }); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index cdad638c6a..be4b59ce08 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -2089,38 +2089,40 @@ describe('ReactDOMInput', () => { it('sets type, step, min, max before value always', () => { const log = []; const originalCreateElement = document.createElement; - spyOnDevAndProd(document, 'createElement').mockImplementation(function ( - type, - ) { - const el = originalCreateElement.apply(this, arguments); - let value = ''; - let typeProp = ''; + spyOnDevAndProd(document, 'createElement').mockImplementation( + function (type) { + const el = originalCreateElement.apply(this, arguments); + let value = ''; + let typeProp = ''; - if (type === 'input') { - Object.defineProperty(el, 'type', { - get: function () { - return typeProp; - }, - set: function (val) { - typeProp = String(val); - log.push('set property type'); - }, - }); - Object.defineProperty(el, 'value', { - get: function () { - return value; - }, - set: function (val) { - value = String(val); - log.push('set property value'); - }, - }); - spyOnDevAndProd(el, 'setAttribute').mockImplementation(function (name) { - log.push('set attribute ' + name); - }); - } - return el; - }); + if (type === 'input') { + Object.defineProperty(el, 'type', { + get: function () { + return typeProp; + }, + set: function (val) { + typeProp = String(val); + log.push('set property type'); + }, + }); + Object.defineProperty(el, 'value', { + get: function () { + return value; + }, + set: function (val) { + value = String(val); + log.push('set property value'); + }, + }); + spyOnDevAndProd(el, 'setAttribute').mockImplementation( + function (name) { + log.push('set attribute ' + name); + }, + ); + } + return el; + }, + ); ReactDOM.render( { const log = []; const originalCreateElement = document.createElement; - spyOnDevAndProd(document, 'createElement').mockImplementation(function ( - type, - ) { - const el = originalCreateElement.apply(this, arguments); - const getDefaultValue = Object.getOwnPropertyDescriptor( - HTMLInputElement.prototype, - 'defaultValue', - ).get; - const setDefaultValue = Object.getOwnPropertyDescriptor( - HTMLInputElement.prototype, - 'defaultValue', - ).set; - const getValue = Object.getOwnPropertyDescriptor( - HTMLInputElement.prototype, - 'value', - ).get; - const setValue = Object.getOwnPropertyDescriptor( - HTMLInputElement.prototype, - 'value', - ).set; - const getType = Object.getOwnPropertyDescriptor( - HTMLInputElement.prototype, - 'type', - ).get; - const setType = Object.getOwnPropertyDescriptor( - HTMLInputElement.prototype, - 'type', - ).set; - if (type === 'input') { - Object.defineProperty(el, 'defaultValue', { - get: function () { - return getDefaultValue.call(this); - }, - set: function (val) { - log.push(`node.defaultValue = ${strify(val)}`); - setDefaultValue.call(this, val); - }, - }); - Object.defineProperty(el, 'value', { - get: function () { - return getValue.call(this); - }, - set: function (val) { - log.push(`node.value = ${strify(val)}`); - setValue.call(this, val); - }, - }); - Object.defineProperty(el, 'type', { - get: function () { - return getType.call(this); - }, - set: function (val) { - log.push(`node.type = ${strify(val)}`); - setType.call(this, val); - }, - }); - spyOnDevAndProd(el, 'setAttribute').mockImplementation(function ( - name, - val, - ) { - log.push(`node.setAttribute(${strify(name)}, ${strify(val)})`); - }); - } - return el; - }); + spyOnDevAndProd(document, 'createElement').mockImplementation( + function (type) { + const el = originalCreateElement.apply(this, arguments); + const getDefaultValue = Object.getOwnPropertyDescriptor( + HTMLInputElement.prototype, + 'defaultValue', + ).get; + const setDefaultValue = Object.getOwnPropertyDescriptor( + HTMLInputElement.prototype, + 'defaultValue', + ).set; + const getValue = Object.getOwnPropertyDescriptor( + HTMLInputElement.prototype, + 'value', + ).get; + const setValue = Object.getOwnPropertyDescriptor( + HTMLInputElement.prototype, + 'value', + ).set; + const getType = Object.getOwnPropertyDescriptor( + HTMLInputElement.prototype, + 'type', + ).get; + const setType = Object.getOwnPropertyDescriptor( + HTMLInputElement.prototype, + 'type', + ).set; + if (type === 'input') { + Object.defineProperty(el, 'defaultValue', { + get: function () { + return getDefaultValue.call(this); + }, + set: function (val) { + log.push(`node.defaultValue = ${strify(val)}`); + setDefaultValue.call(this, val); + }, + }); + Object.defineProperty(el, 'value', { + get: function () { + return getValue.call(this); + }, + set: function (val) { + log.push(`node.value = ${strify(val)}`); + setValue.call(this, val); + }, + }); + Object.defineProperty(el, 'type', { + get: function () { + return getType.call(this); + }, + set: function (val) { + log.push(`node.type = ${strify(val)}`); + setType.call(this, val); + }, + }); + spyOnDevAndProd(el, 'setAttribute').mockImplementation( + function (name, val) { + log.push(`node.setAttribute(${strify(name)}, ${strify(val)})`); + }, + ); + } + return el; + }, + ); ReactDOM.render(, container); diff --git a/packages/react-dom/src/__tests__/ReactDOMTextarea-test.js b/packages/react-dom/src/__tests__/ReactDOMTextarea-test.js index d4cb435688..39a1bac1e9 100644 --- a/packages/react-dom/src/__tests__/ReactDOMTextarea-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMTextarea-test.js @@ -137,24 +137,24 @@ describe('ReactDOMTextarea', () => { let counter = 0; const originalCreateElement = document.createElement; - spyOnDevAndProd(document, 'createElement').mockImplementation(function ( - type, - ) { - const el = originalCreateElement.apply(this, arguments); - let value = ''; - if (type === 'textarea') { - Object.defineProperty(el, 'value', { - get: function () { - return value; - }, - set: function (val) { - value = String(val); - counter++; - }, - }); - } - return el; - }); + spyOnDevAndProd(document, 'createElement').mockImplementation( + function (type) { + const el = originalCreateElement.apply(this, arguments); + let value = ''; + if (type === 'textarea') { + Object.defineProperty(el, 'value', { + get: function () { + return value; + }, + set: function (val) { + value = String(val); + counter++; + }, + }); + } + return el; + }, + ); ReactDOM.render(