From fd2d2799840d9066a752bb32bbbb07c93f64a891 Mon Sep 17 00:00:00 2001 From: lauren Date: Fri, 17 Jan 2025 15:16:36 -0500 Subject: [PATCH 01/19] [eslint-plugin-react-hooks] Inline meta fields (#32115) rollup doesn't inline cjs requires (although it can with an external plugin), so requiring package.json was causing issues internally at Meta since that file doesn't exist there. We could teach our build scripts to do so but given that the eslint meta field is optional anyways I opted to just hardcode the name and omit the version. --- packages/eslint-plugin-react-hooks/src/index.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/src/index.js b/packages/eslint-plugin-react-hooks/src/index.js index c9da639a4f..0ebfbc91a5 100644 --- a/packages/eslint-plugin-react-hooks/src/index.js +++ b/packages/eslint-plugin-react-hooks/src/index.js @@ -10,8 +10,6 @@ import RulesOfHooks from './RulesOfHooks'; import ExhaustiveDeps from './ExhaustiveDeps'; -const {name, version} = require('../package.json'); - // All rules export const rules = { 'rules-of-hooks': RulesOfHooks, @@ -32,7 +30,7 @@ const legacyRecommendedConfig = { // Base plugin object const reactHooksPlugin = { - meta: {name, version}, + meta: {name: 'eslint-plugin-react-hooks'}, rules, }; From 829401dc173d79994a3401fce24084670f55fb5c Mon Sep 17 00:00:00 2001 From: Hendrik Liebau Date: Fri, 17 Jan 2025 23:48:57 +0100 Subject: [PATCH 02/19] [Flight] Transport custom error names in dev mode (#32116) Typed errors is not a feature that Flight currently supports. However, for presentation purposes, serializing a custom error name is something we could support today. With this PR, we're now transporting custom error names through the server-client boundary, so that they are available in the client e.g. for console replaying. One example where this can be useful is when you want to print debug information while leveraging the fact that `console.warn` displays the error stack, including handling of hiding and source mapping stack frames. In this case you may want to show `Warning: ...` or `Debug: ...` instead of `Error: ...`. In prod mode, we still transport an obfuscated error that uses the default `Error` name, to not leak any sensitive information from the server to the client. This also means that you must not rely on the error name to discriminate errors, e.g. when handling them in an error boundary. --- packages/react-client/src/ReactFlightClient.js | 10 +++++++++- .../react-client/src/__tests__/ReactFlight-test.js | 14 ++++++++++++-- packages/react-server/src/ReactFlightServer.js | 4 +++- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 288d6b7c1d..4ea66dcddd 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -2123,8 +2123,15 @@ function resolveErrorProd(response: Response): Error { function resolveErrorDev( response: Response, - errorInfo: {message: string, stack: ReactStackTrace, env: string, ...}, + errorInfo: { + name: string, + message: string, + stack: ReactStackTrace, + env: string, + ... + }, ): Error { + const name: string = errorInfo.name; const message: string = errorInfo.message; const stack: ReactStackTrace = errorInfo.stack; const env: string = errorInfo.env; @@ -2156,6 +2163,7 @@ function resolveErrorDev( error = callStack(); } + (error: any).name = name; (error: any).environmentName = env; return error; } diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index e11c112614..44b0c76c3f 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -694,9 +694,17 @@ describe('ReactFlight', () => { }); it('can transport Error objects as values', async () => { + class CustomError extends Error { + constructor(message) { + super(message); + this.name = 'Custom'; + } + } + function ComponentClient({prop}) { return ` is error: ${prop instanceof Error} + name: ${prop.name} message: ${prop.message} stack: ${normalizeCodeLocInfo(prop.stack).split('\n').slice(0, 2).join('\n')} environmentName: ${prop.environmentName} @@ -705,7 +713,7 @@ describe('ReactFlight', () => { const Component = clientReference(ComponentClient); function ServerComponent() { - const error = new Error('hello'); + const error = new CustomError('hello'); return ; } @@ -718,14 +726,16 @@ describe('ReactFlight', () => { if (__DEV__) { expect(ReactNoop).toMatchRenderedOutput(` is error: true + name: Custom message: hello - stack: Error: hello + stack: Custom: hello in ServerComponent (at **) environmentName: Server `); } else { expect(ReactNoop).toMatchRenderedOutput(` is error: true + name: Error message: An error occurred in the Server Components render. The specific message is omitted in production builds to avoid leaking sensitive details. A digest property is included on this error instance which may provide additional details about the nature of the error. stack: Error: An error occurred in the Server Components render. The specific message is omitted in production builds to avoid leaking sensitive details. A digest property is included on this error instance which may provide additional details about the nature of the error. environmentName: undefined diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 67ae695ca0..cda88fc5c1 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -3093,10 +3093,12 @@ function emitPostponeChunk( function serializeErrorValue(request: Request, error: Error): string { if (__DEV__) { + let name; let message; let stack: ReactStackTrace; let env = (0, request.environmentName)(); try { + name = error.name; // eslint-disable-next-line react-internal/safe-string-coercion message = String(error.message); stack = filterStackTrace(request, error, 0); @@ -3110,7 +3112,7 @@ function serializeErrorValue(request: Request, error: Error): string { message = 'An error occurred but serializing the error message failed.'; stack = []; } - const errorInfo = {message, stack, env}; + const errorInfo = {name, message, stack, env}; const id = outlineModel(request, errorInfo); return '$Z' + id.toString(16); } else { From 18eaf51bd51fed8dfed661d64c306759101d0bfd Mon Sep 17 00:00:00 2001 From: Orta Therox Date: Sat, 18 Jan 2025 22:41:34 +0000 Subject: [PATCH 03/19] Support eslint 8+ flat plugin syntax out of the box for eslint-plugin-react-compiler (#32120) ## Summary The current docs for the react compiler eslint plugin is based on integrating with the old-style eslint config format. This is generally fine, but most plugins (and the [official docs](https://eslint.org/docs/latest/use/configure/configuration-files#configuration-file)) are now describing themselves in the new format. This PR has two changes: - Update the exports to include a "flat configuration" - Adds a README change describing how to handle both configs The solution is semi-based on @guillaumebrunerie's answer in https://github.com/reactwg/react-compiler/discussions/25 mixed with reading the source code for [eslint-plugin-react-refresh](https://github.com/ArnaudBarre/eslint-plugin-react-refresh/blob/main/src/index.ts) ## How did you test this change? I faked this API in the most recent deploy: ![Screenshot 2025-01-18 at 19 58 44](https://github.com/user-attachments/assets/ae0e4bea-fb96-4073-a5f7-c886d087b6af) Then used that in my app: ![Screenshot 2025-01-18 at 20 04 33](https://github.com/user-attachments/assets/21f77158-7535-453a-b988-49cf59d22d71) and get myself some compiler messages: ``` /Users/orta/dev/app/apps/puzzmo.com/src/palette/HoverPopover.tsx 31:37 error Hooks must always be called in a consistent order, and may not be called conditionally. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) react-compiler/react-compiler /Users/orta/dev/app/apps/puzzmo.com/src/components/gameplay/PlayGamePauseOverlay.tsx 33:7 error Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) react-compiler/react-compiler 35:5 error Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) react-compiler/react-compiler ``` --- .../eslint-plugin-react-compiler/README.md | 19 ++++++++++++++++++- .../eslint-plugin-react-compiler/src/index.ts | 14 ++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/compiler/packages/eslint-plugin-react-compiler/README.md b/compiler/packages/eslint-plugin-react-compiler/README.md index 1dc11454f2..cc70679383 100644 --- a/compiler/packages/eslint-plugin-react-compiler/README.md +++ b/compiler/packages/eslint-plugin-react-compiler/README.md @@ -18,7 +18,24 @@ npm install eslint-plugin-react-compiler --save-dev ## Usage -Add `react-compiler` to the plugins section of your `.eslintrc` configuration file. You can omit the `eslint-plugin-` prefix: +### Flat config + +Edit your eslint 8+ config (for example `eslint.config.mjs`) with the recommended configuration: + +```diff ++ import reactCompiler from "eslint-plugin-react-compiler" +import react from "eslint-plugin-react" + +export default [ + // Your existing config + { ...pluginReact.configs.flat.recommended, settings: { react: { version: "detect" } } }, ++ reactCompiler.config.recommended +] +``` + +### Legacy config (`.eslintrc`) + +Add `react-compiler` to the plugins section of your configuration file. You can omit the `eslint-plugin-` prefix: ```json { diff --git a/compiler/packages/eslint-plugin-react-compiler/src/index.ts b/compiler/packages/eslint-plugin-react-compiler/src/index.ts index a734925751..103cdbbbd3 100644 --- a/compiler/packages/eslint-plugin-react-compiler/src/index.ts +++ b/compiler/packages/eslint-plugin-react-compiler/src/index.ts @@ -11,4 +11,18 @@ module.exports = { rules: { 'react-compiler': ReactCompilerRule, }, + configs: { + recommended: { + plugins: { + 'react-compiler': { + rules: { + 'react-compiler': ReactCompilerRule, + }, + }, + }, + rules: { + 'react-compiler/react-compiler': 'error', + }, + }, + }, }; From 028c8e6cf5ce2a87147a7e03e503ce94c7a7a0cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 21 Jan 2025 15:00:02 -0500 Subject: [PATCH 04/19] Add Transition Types (#32105) This adds an isomorphic API to add Transition Types, which represent the cause, to the current Transition. This is currently mainly for View Transitions but as a concept it's broader and we might expand it to more features and object types in the future. ```js import { unstable_addTransitionType as addTransitionType } from 'react'; startTransition(() => { addTransitionType('my-transition-type'); setState(...); }); ``` If multiple transitions get entangled this is additive and all Transition Types are collected. You can also add more than one type to a Transition (hence the `add` prefix). Transition Types are reset after each commit. Meaning that `` revealing after a `startTransition` does not get any View Transition types associated with it. Note that the scoping rules for this is a little "wrong" in this implementation. Ideally it would be scoped to the nearest outer `startTransition` and grouped with any `setState` inside of it. Including Actions. However, since we currently don't have AsyncContext on the client, it would be too easy to drop a Transition Type if there were no other `setState` in the same `await` task. Multiple Transitions are entangled together anyway right now as a result. So this just tracks a global of all pending Transition Types for the next Transition. An inherent tricky bit with this API is that you could update multiple roots. In that case it should ideally be associated with each root. Transition Tracing solves this by associating a Transition with any updates that are later collected but this suffers from the problem mentioned above. Therefore, I just associate Transition Types with one root - the first one to commit. Since the View Transitions across roots are sequential anyway it kind of makes sense that only one really is the cause and the other one is subsequent. Transition Types can be used to apply different animations based on what caused the Transition. You have three different ways to choose from for how to use them: ## CSS It integrates with [View Transition Types](https://www.w3.org/TR/css-view-transitions-2/#active-view-transition-pseudo-examples) so you can match different animations based on CSS scopes: ```css :root:active-view-transition-type(my-transition-type) { &::view-transition-...(...) { ... } } ``` This is kind of a PITA to write though and if you have a CSS library that provide View Transition Classes it's difficult to import those into these scopes. ## Class per Type This PR also adds an object-as-map form that can be passed to all `className` properties: ```js ``` If multiple types match, then they're joined together. If no types match then the special `"default"` entry is used instead. If any type has the value `"none"` then that wins and the ViewTransition is disabled (not assigned a name). These can be combined with `enter`/`exit`/`update`/`layout`/`share` props to match based on kind of trigger and Transition Type. ```js ``` ## Events In addition, you can also observe the types in the View Transition Event callbacks as the second argument. That way you can pick different imperative Animations based on the cause. ```js { if (types.includes('navigation-back')) { ... } else if (types.includes('navigation-forward')) { ... } else { ... } }}> ``` ## Future In the future we might expose types to `useEffect` for more general purpose usage. This would also allow non-View Transition based Animations such as existing libraries to use this same feature to coordinate the same concept. We might also allow richer objects to be passed along here. Only the strings would apply to View Transitions but the imperative code and effects could do something else with them. --- .../view-transition/src/components/App.js | 13 ++++ .../view-transition/src/components/Page.js | 12 +++- .../src/components/Transitions.module.css | 55 +++++++++++++- .../src/client/ReactFiberConfigDOM.js | 4 +- .../src/ReactFiberConfigNative.js | 2 + .../src/createReactNoop.js | 2 + .../src/ReactFiberViewTransitionComponent.js | 71 +++++++++++++++---- .../src/ReactFiberWorkLoop.js | 45 +++++++++--- .../src/ReactFiberConfigTestHost.js | 2 + .../react/index.experimental.development.js | 1 + packages/react/index.experimental.js | 1 + packages/react/index.js | 1 + packages/react/src/ReactClient.js | 2 + .../react/src/ReactSharedInternalsClient.js | 3 + packages/react/src/ReactTransitionType.js | 21 ++++++ 15 files changed, 207 insertions(+), 28 deletions(-) create mode 100644 packages/react/src/ReactTransitionType.js diff --git a/fixtures/view-transition/src/components/App.js b/fixtures/view-transition/src/components/App.js index 028f511107..275e594d87 100644 --- a/fixtures/view-transition/src/components/App.js +++ b/fixtures/view-transition/src/components/App.js @@ -3,6 +3,7 @@ import React, { useLayoutEffect, useEffect, useState, + unstable_addTransitionType as addTransitionType, } from 'react'; import Chrome from './Chrome'; @@ -35,11 +36,23 @@ export default function App({assets, initialURL}) { if (!event.canIntercept) { return; } + const navigationType = event.navigationType; + const previousIndex = window.navigation.currentEntry.index; const newURL = new URL(event.destination.url); event.intercept({ handler() { let promise; startTransition(() => { + addTransitionType('navigation-' + navigationType); + if (navigationType === 'traverse') { + // For traverse types it's useful to distinguish going back or forward. + const nextIndex = event.destination.index; + if (nextIndex > previousIndex) { + addTransitionType('navigation-forward'); + } else if (nextIndex < previousIndex) { + addTransitionType('navigation-back'); + } + } promise = new Promise(resolve => { setRouterState({ url: newURL.pathname + newURL.search, diff --git a/fixtures/view-transition/src/components/Page.js b/fixtures/view-transition/src/components/Page.js index 688688bcd6..40f6032772 100644 --- a/fixtures/view-transition/src/components/Page.js +++ b/fixtures/view-transition/src/components/Page.js @@ -36,7 +36,7 @@ function Component() { export default function Page({url, navigate}) { const show = url === '/?b'; - function onTransition(viewTransition) { + function onTransition(viewTransition, types) { const keyframes = [ {rotate: '0deg', transformOrigin: '30px 8px'}, {rotate: '360deg', transformOrigin: '30px 8px'}, @@ -59,6 +59,16 @@ export default function Page({url, navigate}) {
+ +

{!show ? 'A' : 'B'}

+
+ +

{!show ? 'A' : 'B'}

+
{show ? (
{a} diff --git a/fixtures/view-transition/src/components/Transitions.module.css b/fixtures/view-transition/src/components/Transitions.module.css index 58f805eff8..8c7b66b446 100644 --- a/fixtures/view-transition/src/components/Transitions.module.css +++ b/fixtures/view-transition/src/components/Transitions.module.css @@ -9,7 +9,18 @@ } } -@keyframes exit-slide-left { +@keyframes enter-slide-left { + 0% { + opacity: 0; + translate: 200px 0; + } + 100% { + opacity: 1; + translate: 0 0; + } +} + +@keyframes exit-slide-right { 0% { opacity: 1; translate: 0 0; @@ -20,9 +31,51 @@ } } +@keyframes exit-slide-left { + 0% { + opacity: 1; + translate: 0 0; + } + 100% { + opacity: 0; + translate: -200px 0; + } +} + +::view-transition-new(.slide-right) { + animation: enter-slide-right ease-in 0.25s; +} +::view-transition-old(.slide-right) { + animation: exit-slide-right ease-in 0.25s; +} +::view-transition-new(.slide-left) { + animation: enter-slide-left ease-in 0.25s; +} +::view-transition-old(.slide-left) { + animation: exit-slide-left ease-in 0.25s; +} + ::view-transition-new(.enter-slide-right):only-child { animation: enter-slide-right ease-in 0.25s; } ::view-transition-old(.exit-slide-left):only-child { animation: exit-slide-left ease-in 0.25s; } + +:root:active-view-transition-type(navigation-back) { + &::view-transition-new(.slide-on-nav) { + animation: enter-slide-right ease-in 0.25s; + } + &::view-transition-old(.slide-on-nav) { + animation: exit-slide-right ease-in 0.25s; + } +} + +:root:active-view-transition-type(navigation-forward) { + &::view-transition-new(.slide-on-nav) { + animation: enter-slide-left ease-in 0.25s; + } + &::view-transition-old(.slide-on-nav) { + animation: exit-slide-left ease-in 0.25s; + } +} diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index cea03b73b5..42cf0a8f84 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -25,6 +25,7 @@ import type { PreinitScriptOptions, PreinitModuleScriptOptions, } from 'react-dom/src/shared/ReactDOMTypes'; +import type {TransitionTypes} from 'react/src/ReactTransitionType.js'; import {NotPending} from '../shared/ReactDOMFormActions'; @@ -1235,6 +1236,7 @@ const SUSPENSEY_FONT_TIMEOUT = 500; export function startViewTransition( rootContainer: Container, + transitionTypes: null | TransitionTypes, mutationCallback: () => void, layoutCallback: () => void, afterMutationCallback: () => void, @@ -1293,7 +1295,7 @@ export function startViewTransition( afterMutationCallback(); } }, - types: null, // TODO: Provide types. + types: transitionTypes, }); // $FlowFixMe[prop-missing] ownerDocument.__reactViewTransition = transition; diff --git a/packages/react-native-renderer/src/ReactFiberConfigNative.js b/packages/react-native-renderer/src/ReactFiberConfigNative.js index 9bbe8b962a..f6709cab03 100644 --- a/packages/react-native-renderer/src/ReactFiberConfigNative.js +++ b/packages/react-native-renderer/src/ReactFiberConfigNative.js @@ -8,6 +8,7 @@ */ import type {InspectorData, TouchedViewDataAtPoint} from './ReactNativeTypes'; +import type {TransitionTypes} from 'react/src/ReactTransitionType.js'; // Modules provided by RN: import { @@ -582,6 +583,7 @@ export function hasInstanceAffectedParent( export function startViewTransition( rootContainer: Container, + transitionTypes: null | TransitionTypes, mutationCallback: () => void, layoutCallback: () => void, afterMutationCallback: () => void, diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index b1b4a3c940..f412f90f6c 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -22,6 +22,7 @@ import type {UpdateQueue} from 'react-reconciler/src/ReactFiberClassUpdateQueue' import type {ReactNodeList} from 'shared/ReactTypes'; import type {RootTag} from 'react-reconciler/src/ReactRootTags'; import type {EventPriority} from 'react-reconciler/src/ReactEventPriorities'; +import type {TransitionTypes} from 'react/src/ReactTransitionType.js'; import * as Scheduler from 'scheduler/unstable_mock'; import {REACT_FRAGMENT_TYPE, REACT_ELEMENT_TYPE} from 'shared/ReactSymbols'; @@ -780,6 +781,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { startViewTransition( rootContainer: Container, + transitionTypes: null | TransitionTypes, mutationCallback: () => void, afterMutationCallback: () => void, layoutCallback: () => void, diff --git a/packages/react-reconciler/src/ReactFiberViewTransitionComponent.js b/packages/react-reconciler/src/ReactFiberViewTransitionComponent.js index 65e3ffc459..16b5ef3049 100644 --- a/packages/react-reconciler/src/ReactFiberViewTransitionComponent.js +++ b/packages/react-reconciler/src/ReactFiberViewTransitionComponent.js @@ -11,26 +11,35 @@ import type {ReactNodeList} from 'shared/ReactTypes'; import type {FiberRoot} from './ReactInternalTypes'; import type {ViewTransitionInstance} from './ReactFiberConfig'; -import {getWorkInProgressRoot} from './ReactFiberWorkLoop'; +import { + getWorkInProgressRoot, + getPendingTransitionTypes, +} from './ReactFiberWorkLoop'; import {getIsHydrating} from './ReactFiberHydrationContext'; import {getTreeId} from './ReactFiberTreeContext'; +export type ViewTransitionClassPerType = { + [transitionType: 'default' | string]: 'none' | string, +}; + +export type ViewTransitionClass = 'none' | string | ViewTransitionClassPerType; + export type ViewTransitionProps = { name?: string, children?: ReactNodeList, - className?: 'none' | string, - enter?: 'none' | string, - exit?: 'none' | string, - layout?: 'none' | string, - share?: 'none' | string, - update?: 'none' | string, - onEnter?: (instance: ViewTransitionInstance) => void, - onExit?: (instance: ViewTransitionInstance) => void, - onLayout?: (instance: ViewTransitionInstance) => void, - onShare?: (instance: ViewTransitionInstance) => void, - onUpdate?: (instance: ViewTransitionInstance) => void, + className?: ViewTransitionClass, + enter?: ViewTransitionClass, + exit?: ViewTransitionClass, + layout?: ViewTransitionClass, + share?: ViewTransitionClass, + update?: ViewTransitionClass, + onEnter?: (instance: ViewTransitionInstance, types: Array) => void, + onExit?: (instance: ViewTransitionInstance, types: Array) => void, + onLayout?: (instance: ViewTransitionInstance, types: Array) => void, + onShare?: (instance: ViewTransitionInstance, types: Array) => void, + onUpdate?: (instance: ViewTransitionInstance, types: Array) => void, }; export type ViewTransitionState = { @@ -82,17 +91,49 @@ export function getViewTransitionName( return (instance.autoName: any); } +function getClassNameByType(classByType: ?ViewTransitionClass): ?string { + if (classByType == null || typeof classByType === 'string') { + return classByType; + } + let className: ?string = null; + const activeTypes = getPendingTransitionTypes(); + if (activeTypes !== null) { + for (let i = 0; i < activeTypes.length; i++) { + const match = classByType[activeTypes[i]]; + if (match != null) { + if (match === 'none') { + // If anything matches "none" that takes precedence over any other + // type that also matches. + return 'none'; + } + if (className == null) { + className = match; + } else { + className += ' ' + match; + } + } + } + } + if (className == null) { + // We had no other matches. Match the default for this configuration. + return classByType.default; + } + return className; +} + export function getViewTransitionClassName( - className: ?string, - eventClassName: ?string, + defaultClass: ?ViewTransitionClass, + eventClass: ?ViewTransitionClass, ): ?string { + const className: ?string = getClassNameByType(defaultClass); + const eventClassName: ?string = getClassNameByType(eventClass); if (eventClassName == null) { return className; } if (eventClassName === 'none') { return eventClassName; } - if (className != null) { + if (className != null && className !== 'none') { return className + ' ' + eventClassName; } return eventClassName; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 43cba4586f..03f09c1dba 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -27,6 +27,7 @@ import { getViewTransitionName, type ViewTransitionState, } from './ReactFiberViewTransitionComponent'; +import type {TransitionTypes} from 'react/src/ReactTransitionType.js'; import { enableCreateEventHandleAPI, @@ -653,7 +654,9 @@ let pendingEffectsRemainingLanes: Lanes = NoLanes; let pendingEffectsRenderEndTime: number = -0; // Profiling-only let pendingPassiveTransitions: Array | null = null; let pendingRecoverableErrors: null | Array> = null; -let pendingViewTransitionEvents: Array<() => void> | null = null; +let pendingViewTransitionEvents: Array<(types: Array) => void> | null = + null; +let pendingTransitionTypes: null | TransitionTypes = null; let pendingDidIncludeRenderPhaseUpdate: boolean = false; let pendingSuspendedCommitReason: SuspendedCommitReason = IMMEDIATE_COMMIT; // Profiling-only @@ -695,6 +698,10 @@ export function getPendingPassiveEffectsLanes(): Lanes { return pendingEffectsLanes; } +export function getPendingTransitionTypes(): null | TransitionTypes { + return pendingTransitionTypes; +} + export function isWorkLoopSuspendedOnData(): boolean { return ( workInProgressSuspendedReason === SuspendedOnData || @@ -804,7 +811,7 @@ export function requestDeferredLane(): Lane { export function scheduleViewTransitionEvent( fiber: Fiber, - callback: ?(instance: ViewTransitionInstance) => void, + callback: ?(instance: ViewTransitionInstance, types: Array) => void, ): void { if (enableViewTransition) { if (callback != null) { @@ -3348,9 +3355,6 @@ function commitRoot( pendingEffectsRemainingLanes = remainingLanes; pendingPassiveTransitions = transitions; pendingRecoverableErrors = recoverableErrors; - if (enableViewTransition) { - pendingViewTransitionEvents = null; - } pendingDidIncludeRenderPhaseUpdate = didIncludeRenderPhaseUpdate; if (enableProfilerTimer) { pendingEffectsRenderEndTime = completedRenderEndTime; @@ -3362,10 +3366,24 @@ function commitRoot( // might get scheduled in the commit phase. (See #16714.) // TODO: Delete all other places that schedule the passive effect callback // They're redundant. - const passiveSubtreeMask = - enableViewTransition && includesOnlyViewTransitionEligibleLanes(lanes) - ? PassiveTransitionMask - : PassiveMask; + let passiveSubtreeMask; + if (enableViewTransition) { + pendingViewTransitionEvents = null; + if (includesOnlyViewTransitionEligibleLanes(lanes)) { + // Claim any pending Transition Types for this commit. + // This means that multiple roots committing independent View Transitions + // 1) end up staggered because we can only have one at a time. + // 2) only the first one gets all the Transition Types. + pendingTransitionTypes = ReactSharedInternals.V; + ReactSharedInternals.V = null; + passiveSubtreeMask = PassiveTransitionMask; + } else { + pendingTransitionTypes = null; + passiveSubtreeMask = PassiveMask; + } + } else { + passiveSubtreeMask = PassiveMask; + } if ( // If this subtree rendered with profiling this commit, we need to visit it to log it. (enableProfilerTimer && @@ -3461,6 +3479,7 @@ function commitRoot( shouldStartViewTransition && startViewTransition( root.containerInfo, + pendingTransitionTypes, flushMutationEffects, flushLayoutEffects, flushAfterMutationEffects, @@ -3708,11 +3727,17 @@ function flushSpawnedWork(): void { // effects or spawned sync work since this is still part of the previous commit. // Even though conceptually it's like its own task between layout effets and passive. const pendingEvents = pendingViewTransitionEvents; + let pendingTypes = pendingTransitionTypes; + pendingTransitionTypes = null; if (pendingEvents !== null) { pendingViewTransitionEvents = null; + if (pendingTypes === null) { + // Normalize the type. This is lazily created only for events. + pendingTypes = []; + } for (let i = 0; i < pendingEvents.length; i++) { const viewTransitionEvent = pendingEvents[i]; - viewTransitionEvent(); + viewTransitionEvent(pendingTypes); } } } diff --git a/packages/react-test-renderer/src/ReactFiberConfigTestHost.js b/packages/react-test-renderer/src/ReactFiberConfigTestHost.js index a33590a32b..aa8b34523b 100644 --- a/packages/react-test-renderer/src/ReactFiberConfigTestHost.js +++ b/packages/react-test-renderer/src/ReactFiberConfigTestHost.js @@ -8,6 +8,7 @@ */ import type {ReactContext} from 'shared/ReactTypes'; +import type {TransitionTypes} from 'react/src/ReactTransitionType.js'; import isArray from 'shared/isArray'; import {REACT_CONTEXT_TYPE} from 'shared/ReactSymbols'; @@ -364,6 +365,7 @@ export function hasInstanceAffectedParent( export function startViewTransition( rootContainer: Container, + transitionTypes: null | TransitionTypes, mutationCallback: () => void, layoutCallback: () => void, afterMutationCallback: () => void, diff --git a/packages/react/index.experimental.development.js b/packages/react/index.experimental.development.js index 53d7cd55b9..49c98bb208 100644 --- a/packages/react/index.experimental.development.js +++ b/packages/react/index.experimental.development.js @@ -33,6 +33,7 @@ export { unstable_getCacheForType, unstable_SuspenseList, unstable_ViewTransition, + unstable_addTransitionType, unstable_useCacheRefresh, useId, useCallback, diff --git a/packages/react/index.experimental.js b/packages/react/index.experimental.js index 2efbac3b7d..77cf6bd0e0 100644 --- a/packages/react/index.experimental.js +++ b/packages/react/index.experimental.js @@ -33,6 +33,7 @@ export { unstable_getCacheForType, unstable_SuspenseList, unstable_ViewTransition, + unstable_addTransitionType, unstable_useCacheRefresh, useId, useCallback, diff --git a/packages/react/index.js b/packages/react/index.js index 7522a32d18..711a044455 100644 --- a/packages/react/index.js +++ b/packages/react/index.js @@ -52,6 +52,7 @@ export { unstable_SuspenseList, unstable_TracingMarker, unstable_ViewTransition, + unstable_addTransitionType, unstable_getCacheForType, unstable_useCacheRefresh, useId, diff --git a/packages/react/src/ReactClient.js b/packages/react/src/ReactClient.js index 4027534f71..f633c7617d 100644 --- a/packages/react/src/ReactClient.js +++ b/packages/react/src/ReactClient.js @@ -61,6 +61,7 @@ import { } from './ReactHooks'; import ReactSharedInternals from './ReactSharedInternalsClient'; import {startTransition} from './ReactStartTransition'; +import {addTransitionType} from './ReactTransitionType'; import {act} from './ReactAct'; import {captureOwnerStack} from './ReactOwnerStack'; import * as ReactCompilerRuntime from './ReactCompilerRuntime'; @@ -126,6 +127,7 @@ export { REACT_TRACING_MARKER_TYPE as unstable_TracingMarker, // enableViewTransition REACT_VIEW_TRANSITION_TYPE as unstable_ViewTransition, + addTransitionType as unstable_addTransitionType, useId, act, // DEV-only captureOwnerStack, // DEV-only diff --git a/packages/react/src/ReactSharedInternalsClient.js b/packages/react/src/ReactSharedInternalsClient.js index 452bd933da..f899cba24a 100644 --- a/packages/react/src/ReactSharedInternalsClient.js +++ b/packages/react/src/ReactSharedInternalsClient.js @@ -10,12 +10,14 @@ import type {Dispatcher} from 'react-reconciler/src/ReactInternalTypes'; import type {AsyncDispatcher} from 'react-reconciler/src/ReactInternalTypes'; import type {BatchConfigTransition} from 'react-reconciler/src/ReactFiberTracingMarkerComponent'; +import type {TransitionTypes} from './ReactTransitionType'; export type SharedStateClient = { H: null | Dispatcher, // ReactCurrentDispatcher for Hooks A: null | AsyncDispatcher, // ReactCurrentCache for Cache T: null | BatchConfigTransition, // ReactCurrentBatchConfig for Transitions S: null | ((BatchConfigTransition, mixed) => void), // onStartTransitionFinish + V: null | TransitionTypes, // Pending Transition Types for the Next Transition // DEV-only @@ -45,6 +47,7 @@ const ReactSharedInternals: SharedStateClient = ({ A: null, T: null, S: null, + V: null, }: any); if (__DEV__) { diff --git a/packages/react/src/ReactTransitionType.js b/packages/react/src/ReactTransitionType.js new file mode 100644 index 0000000000..c0c1a3a216 --- /dev/null +++ b/packages/react/src/ReactTransitionType.js @@ -0,0 +1,21 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import ReactSharedInternals from 'shared/ReactSharedInternals'; + +export type TransitionTypes = Array; + +export function addTransitionType(type: string): void { + const pendingTransitionTypes: null | TransitionTypes = ReactSharedInternals.V; + if (pendingTransitionTypes === null) { + ReactSharedInternals.V = [type]; + } else if (pendingTransitionTypes.indexOf(type) === -1) { + pendingTransitionTypes.push(type); + } +} From b000019578a417ec0a1aeec8bda689db240cb28e Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Wed, 22 Jan 2025 14:15:48 +0000 Subject: [PATCH 05/19] DevTools: support useEffectEvent and forward-fix experimental prefix support (#32106) - Adds support for `experimental_useEffectEvent`, now DevTools will be able to display this hook for inspected element - Added a use case to DevTools shell, couldn't add case, because we are using ReactTestRenderer, which has the corresponding flag disabled. - Forward-fix logic for handling `experimental` prefix that was added in https://github.com/facebook/react/pull/32088. ![Screenshot 2025-01-16 at 21 24 12](https://github.com/user-attachments/assets/6fb8ff2a-be47-47b5-bbfc-73d3a586657c) --- .../react-debug-tools/src/ReactDebugHooks.js | 24 +++++++++++++- .../InspectableElements.js | 2 ++ .../app/InspectableElements/UseEffectEvent.js | 32 +++++++++++++++++++ 3 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 packages/react-devtools-shell/src/app/InspectableElements/UseEffectEvent.js diff --git a/packages/react-debug-tools/src/ReactDebugHooks.js b/packages/react-debug-tools/src/ReactDebugHooks.js index 2e1fcb19a6..f30489b7f6 100644 --- a/packages/react-debug-tools/src/ReactDebugHooks.js +++ b/packages/react-debug-tools/src/ReactDebugHooks.js @@ -127,6 +127,13 @@ function getPrimitiveStackCache(): Map> { } Dispatcher.useId(); + + if (typeof Dispatcher.useResourceEffect === 'function') { + Dispatcher.useResourceEffect(() => ({}), []); + } + if (typeof Dispatcher.useEffectEvent === 'function') { + Dispatcher.useEffectEvent((args: empty) => {}); + } } finally { readHookLog = hookLog; hookLog = []; @@ -749,6 +756,20 @@ function useResourceEffect( }); } +function useEffectEvent) => mixed>(callback: F): F { + nextHook(); + hookLog.push({ + displayName: null, + primitive: 'EffectEvent', + stackError: new Error(), + value: callback, + debugInfo: null, + dispatcherHookName: 'EffectEvent', + }); + + return callback; +} + const Dispatcher: DispatcherType = { use, readContext, @@ -773,6 +794,7 @@ const Dispatcher: DispatcherType = { useFormState, useActionState, useHostTransitionStatus, + useEffectEvent, useResourceEffect, }; @@ -962,7 +984,7 @@ function parseHookName(functionName: void | string): string { startIndex += 'unstable_'.length; } - if (functionName.slice(startIndex).startsWith('unstable_')) { + if (functionName.slice(startIndex).startsWith('experimental_')) { startIndex += 'experimental_'.length; } diff --git a/packages/react-devtools-shell/src/app/InspectableElements/InspectableElements.js b/packages/react-devtools-shell/src/app/InspectableElements/InspectableElements.js index abc6eee336..95f104925b 100644 --- a/packages/react-devtools-shell/src/app/InspectableElements/InspectableElements.js +++ b/packages/react-devtools-shell/src/app/InspectableElements/InspectableElements.js @@ -19,6 +19,7 @@ import NestedProps from './NestedProps'; import SimpleValues from './SimpleValues'; import SymbolKeys from './SymbolKeys'; import UseMemoCache from './UseMemoCache'; +import UseEffectEvent from './UseEffectEvent'; // TODO Add Immutable JS example @@ -36,6 +37,7 @@ export default function InspectableElements(): React.Node { + ); } diff --git a/packages/react-devtools-shell/src/app/InspectableElements/UseEffectEvent.js b/packages/react-devtools-shell/src/app/InspectableElements/UseEffectEvent.js new file mode 100644 index 0000000000..e55f6a3c55 --- /dev/null +++ b/packages/react-devtools-shell/src/app/InspectableElements/UseEffectEvent.js @@ -0,0 +1,32 @@ +import * as React from 'react'; + +const {experimental_useEffectEvent, useState, useEffect} = React; + +export default function UseEffectEvent(): React.Node { + return ( + <> + + + + ); +} + +function SingleHookCase() { + const onClick = experimental_useEffectEvent(() => {}); + + return
; +} + +function useCustomHook() { + const [state, setState] = useState(); + const onClick = experimental_useEffectEvent(() => {}); + useEffect(() => {}); + + return [state, setState, onClick]; +} + +function HookTreeCase() { + const onClick = useCustomHook(); + + return
; +} From 5f05181a8b527260fc0a338edcde0e9d3b35ab20 Mon Sep 17 00:00:00 2001 From: "Sebastian \"Sebbie\" Silbermann" Date: Wed, 22 Jan 2025 16:39:00 +0100 Subject: [PATCH 06/19] Include error name in error chunks (#32157) --- packages/react-client/src/ReactFlightClient.js | 17 ++++++----------- .../src/__tests__/ReactFlight-test.js | 1 + packages/react-server/src/ReactFlightServer.js | 16 ++++++++++------ packages/shared/ReactTypes.js | 14 ++++++++++++++ 4 files changed, 31 insertions(+), 17 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 4ea66dcddd..3d125cc310 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -16,6 +16,7 @@ import type { ReactTimeInfo, ReactStackTrace, ReactCallSite, + ReactErrorInfoDev, } from 'shared/ReactTypes'; import type {LazyComponent} from 'react/src/ReactLazy'; @@ -2123,18 +2124,12 @@ function resolveErrorProd(response: Response): Error { function resolveErrorDev( response: Response, - errorInfo: { - name: string, - message: string, - stack: ReactStackTrace, - env: string, - ... - }, + errorInfo: ReactErrorInfoDev, ): Error { - const name: string = errorInfo.name; - const message: string = errorInfo.message; - const stack: ReactStackTrace = errorInfo.stack; - const env: string = errorInfo.env; + const name = errorInfo.name; + const message = errorInfo.message; + const stack = errorInfo.stack; + const env = errorInfo.env; if (!__DEV__) { // These errors should never make it into a build so we don't need to encode them in codes.json diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index 44b0c76c3f..25bbd0e83a 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -1387,6 +1387,7 @@ describe('ReactFlight', () => { errors: [ { message: 'This is an error', + name: 'Error', stack: expect.stringContaining( 'Error: This is an error\n' + ' at eval (eval at testFunction (inspected-page.html:29:11),%20%3Canonymous%3E:1:35)\n' + diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index cda88fc5c1..0c7d12219c 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -64,6 +64,8 @@ import type { ReactTimeInfo, ReactStackTrace, ReactCallSite, + ReactErrorInfo, + ReactErrorInfoDev, } from 'shared/ReactTypes'; import type {ReactElement} from 'shared/ReactElementType'; import type {LazyComponent} from 'react/src/ReactLazy'; @@ -3093,8 +3095,8 @@ function emitPostponeChunk( function serializeErrorValue(request: Request, error: Error): string { if (__DEV__) { - let name; - let message; + let name: string = 'Error'; + let message: string; let stack: ReactStackTrace; let env = (0, request.environmentName)(); try { @@ -3112,7 +3114,7 @@ function serializeErrorValue(request: Request, error: Error): string { message = 'An error occurred but serializing the error message failed.'; stack = []; } - const errorInfo = {name, message, stack, env}; + const errorInfo: ReactErrorInfoDev = {name, message, stack, env}; const id = outlineModel(request, errorInfo); return '$Z' + id.toString(16); } else { @@ -3129,13 +3131,15 @@ function emitErrorChunk( digest: string, error: mixed, ): void { - let errorInfo: any; + let errorInfo: ReactErrorInfo; if (__DEV__) { - let message; + let name: string = 'Error'; + let message: string; let stack: ReactStackTrace; let env = (0, request.environmentName)(); try { if (error instanceof Error) { + name = error.name; // eslint-disable-next-line react-internal/safe-string-coercion message = String(error.message); stack = filterStackTrace(request, error, 0); @@ -3157,7 +3161,7 @@ function emitErrorChunk( message = 'An error occurred but serializing the error message failed.'; stack = []; } - errorInfo = {digest, message, stack, env}; + errorInfo = {digest, name, message, stack, env}; } else { errorInfo = {digest}; } diff --git a/packages/shared/ReactTypes.js b/packages/shared/ReactTypes.js index 26cd57fc96..735f96a36f 100644 --- a/packages/shared/ReactTypes.js +++ b/packages/shared/ReactTypes.js @@ -203,6 +203,20 @@ export type ReactEnvironmentInfo = { +env: string, }; +export type ReactErrorInfoProd = { + +digest: string, +}; + +export type ReactErrorInfoDev = { + +digest?: string, + +name: string, + +message: string, + +stack: ReactStackTrace, + +env: string, +}; + +export type ReactErrorInfo = ReactErrorInfoProd | ReactErrorInfoDev; + export type ReactAsyncInfo = { +type: string, // Stashed Data for the Specific Execution Environment. Not part of the transport protocol From 9b62ee71f483502db4f44220552b97757b346094 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=97=8D+85CD?= <50108258+kwaa@users.noreply.github.com> Date: Wed, 22 Jan 2025 23:59:50 +0800 Subject: [PATCH 07/19] docs(eslint-plugin-react-compiler): fix typo (#32149) --- compiler/packages/eslint-plugin-react-compiler/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/packages/eslint-plugin-react-compiler/README.md b/compiler/packages/eslint-plugin-react-compiler/README.md index cc70679383..3bf175f85e 100644 --- a/compiler/packages/eslint-plugin-react-compiler/README.md +++ b/compiler/packages/eslint-plugin-react-compiler/README.md @@ -29,7 +29,7 @@ import react from "eslint-plugin-react" export default [ // Your existing config { ...pluginReact.configs.flat.recommended, settings: { react: { version: "detect" } } }, -+ reactCompiler.config.recommended ++ reactCompiler.configs.recommended ] ``` From e5a2062c80abe2118b8bd32972a5100a2b1ffa01 Mon Sep 17 00:00:00 2001 From: Alex Yang Date: Wed, 22 Jan 2025 09:04:59 -0800 Subject: [PATCH 08/19] fix(react-compiler): `JSXText` emits incorrect with bracket (#32138) ## Summary Our [LlamaIndex](https://www.llamaindex.ai/) Product is blocked by this bug Fixes: https://github.com/facebook/react/issues/32137 ## How did you test this change? --- .../ReactiveScopes/CodegenReactiveFunction.ts | 2 +- .../compiler/jsx-bracket-in-text.expect.md | 51 +++++++++++++++++++ .../fixtures/compiler/jsx-bracket-in-text.jsx | 13 +++++ 3 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-bracket-in-text.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-bracket-in-text.jsx diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts index b9ec688d87..76135f96b4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -2327,7 +2327,7 @@ function codegenJsxAttribute( } } -const JSX_TEXT_CHILD_REQUIRES_EXPR_CONTAINER_PATTERN = /[<>&]/; +const JSX_TEXT_CHILD_REQUIRES_EXPR_CONTAINER_PATTERN = /[<>&{}]/; function codegenJsxElement( cx: Context, place: Place, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-bracket-in-text.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-bracket-in-text.expect.md new file mode 100644 index 0000000000..21a2f31ccd --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-bracket-in-text.expect.md @@ -0,0 +1,51 @@ + +## Input + +```javascript +function Test() { + return ( +
+ If the string contains the string {pageNumber} it will be + replaced by the page number. +
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Test, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function Test() { + const $ = _c(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = ( +
+ { + "If the string contains the string {pageNumber} it will be replaced by the page number." + } +
+ ); + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Test, + params: [], +}; + +``` + +### Eval output +(kind: ok)
If the string contains the string {pageNumber} it will be replaced by the page number.
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-bracket-in-text.jsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-bracket-in-text.jsx new file mode 100644 index 0000000000..1e93f5e0ad --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-bracket-in-text.jsx @@ -0,0 +1,13 @@ +function Test() { + return ( +
+ If the string contains the string {pageNumber} it will be + replaced by the page number. +
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Test, + params: [], +}; From 19557443c8c2f54571dbb1519403cf310ad6e68b Mon Sep 17 00:00:00 2001 From: mofeiZ <34200447+mofeiZ@users.noreply.github.com> Date: Wed, 22 Jan 2025 13:52:05 -0500 Subject: [PATCH 09/19] [compiler][repro] JSX escape sequences not printed correctly by @babel/generator (#32130) Repro for https://github.com/facebook/react/issues/32123 Note that this is only a bug when calling `@babel/generator:generate()` before transforming JSX. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32130). * #32131 * __->__ #32130 --- .../jsx-preserve-escape-character.expect.md | 57 +++++++++++++++++++ .../compiler/jsx-preserve-escape-character.js | 17 ++++++ 2 files changed, 74 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-preserve-escape-character.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-preserve-escape-character.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-preserve-escape-character.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-preserve-escape-character.expect.md new file mode 100644 index 0000000000..6f8a86382d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-preserve-escape-character.expect.md @@ -0,0 +1,57 @@ + +## Input + +```javascript +/** + * Fixture showing `@babel/generator` bug with jsx attribute strings containing + * escape sequences. Note that this is only a problem when generating jsx + * literals. + * + * When using the jsx transform to correctly lower jsx into + * `React.createElement` calls, the escape sequences are preserved correctly + * (see evaluator output). + */ +function MyApp() { + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: MyApp, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; /** + * Fixture showing `@babel/generator` bug with jsx attribute strings containing + * escape sequences. Note that this is only a problem when generating jsx + * literals. + * + * When using the jsx transform to correctly lower jsx into + * `React.createElement` calls, the escape sequences are preserved correctly + * (see evaluator output). + */ +function MyApp() { + const $ = _c(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = ; + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: MyApp, + params: [], +}; + +``` + +### Eval output +(kind: ok) \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-preserve-escape-character.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-preserve-escape-character.js new file mode 100644 index 0000000000..5a972a585b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-preserve-escape-character.js @@ -0,0 +1,17 @@ +/** + * Fixture showing `@babel/generator` bug with jsx attribute strings containing + * escape sequences. Note that this is only a problem when generating jsx + * literals. + * + * When using the jsx transform to correctly lower jsx into + * `React.createElement` calls, the escape sequences are preserved correctly + * (see evaluator output). + */ +function MyApp() { + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: MyApp, + params: [], +}; From 7c864c98342e6e92a992ac32c1846f13eb1a314c Mon Sep 17 00:00:00 2001 From: mofeiZ <34200447+mofeiZ@users.noreply.github.com> Date: Wed, 22 Jan 2025 14:22:35 -0500 Subject: [PATCH 10/19] [compiler][ez] Patch for JSX escape sequences in @babel/generator (#32131) Fall back to using JSXExpressionContainer for strings potentially containing escape sequences (a single backslash) to fix https://github.com/facebook/react/issues/32123. This is an extension of https://github.com/facebook/react/pull/29079 --- .../src/ReactiveScopes/CodegenReactiveFunction.ts | 2 +- .../jsx-preserve-escape-character.expect.md | 2 +- ...epro-propagate-type-of-ternary-nested.expect.md | 14 +++++++++++--- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts index 76135f96b4..cf40d86abc 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -2269,7 +2269,7 @@ function codegenInstructionValue( * https://en.wikipedia.org/wiki/List_of_Unicode_characters#Control_codes */ const STRING_REQUIRES_EXPR_CONTAINER_PATTERN = - /[\u{0000}-\u{001F}\u{007F}\u{0080}-\u{FFFF}]|"/u; + /[\u{0000}-\u{001F}\u{007F}\u{0080}-\u{FFFF}]|"|\\/u; function codegenJsxAttribute( cx: Context, attribute: JsxAttribute, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-preserve-escape-character.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-preserve-escape-character.expect.md index 6f8a86382d..a539d92ed9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-preserve-escape-character.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-preserve-escape-character.expect.md @@ -38,7 +38,7 @@ function MyApp() { const $ = _c(1); let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 = ; + t0 = ; $[0] = t0; } else { t0 = $[0]; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-nested.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-nested.expect.md index 609231ca8d..97109cfc4e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-nested.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-propagate-type-of-ternary-nested.expect.md @@ -42,15 +42,23 @@ function V0(t0) { gmhubcw {v1 === V3.V13 ? ( - + iawyneijcgamsfgrrjyvhjrrqvzexxwenxqoknnilmfloafyvnvkqbssqnxnexqvtcpvjysaiovjxyqrorqskfph ) : v16.v17("pyorztRC]EJzVuP^e") ? ( - + goprinbjmmjhfserfuqyluxcewpyjihektogc ) : ( - + yejarlvudihqdrdgpvahovggdnmgnueedxpbwbkdvvkdhqwrtoiual )} From b6b33bfb92c095160df7370fb488acb89c55b5ca Mon Sep 17 00:00:00 2001 From: mofeiZ <34200447+mofeiZ@users.noreply.github.com> Date: Wed, 22 Jan 2025 14:34:08 -0500 Subject: [PATCH 11/19] [compiler][ez] rewrite invariant in InferReferenceEffects (#32093) Small patch to pass aliased context values into `Object|ArrayExpression`s --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32093). * #32099 * #32104 * #32098 * #32097 * #32096 * #32095 * #32094 * __->__ #32093 --- .../src/Inference/InferFunctionEffects.ts | 7 +- .../src/Inference/InferReferenceEffects.ts | 75 +++++++++++-------- 2 files changed, 49 insertions(+), 33 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts index 0ae54839b6..fe209924e4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts @@ -41,11 +41,16 @@ function inferOperandEffect(state: State, place: Place): null | FunctionEffect { if (isRefOrRefValue(place.identifier)) { break; } else if (value.kind === ValueKind.Context) { + CompilerError.invariant(value.context.size > 0, { + reason: + "[InferFunctionEffects] Expected Context-kind value's capture list to be non-empty.", + loc: place.loc, + }); return { kind: 'ContextMutation', loc: place.loc, effect: place.effect, - places: value.context.size === 0 ? new Set([place]) : value.context, + places: value.context, }; } else if ( value.kind !== ValueKind.Mutable && diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts index 8cf30a9666..70395e58d9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts @@ -857,17 +857,19 @@ function inferBlock( break; } case 'ArrayExpression': { - const valueKind: AbstractValue = hasContextRefOperand(state, instrValue) - ? { - kind: ValueKind.Context, - reason: new Set([ValueReason.Other]), - context: new Set(), - } - : { - kind: ValueKind.Mutable, - reason: new Set([ValueReason.Other]), - context: new Set(), - }; + const contextRefOperands = getContextRefOperand(state, instrValue); + const valueKind: AbstractValue = + contextRefOperands.length > 0 + ? { + kind: ValueKind.Context, + reason: new Set([ValueReason.Other]), + context: new Set(contextRefOperands), + } + : { + kind: ValueKind.Mutable, + reason: new Set([ValueReason.Other]), + context: new Set(), + }; continuation = { kind: 'initialize', valueKind, @@ -918,17 +920,19 @@ function inferBlock( break; } case 'ObjectExpression': { - const valueKind: AbstractValue = hasContextRefOperand(state, instrValue) - ? { - kind: ValueKind.Context, - reason: new Set([ValueReason.Other]), - context: new Set(), - } - : { - kind: ValueKind.Mutable, - reason: new Set([ValueReason.Other]), - context: new Set(), - }; + const contextRefOperands = getContextRefOperand(state, instrValue); + const valueKind: AbstractValue = + contextRefOperands.length > 0 + ? { + kind: ValueKind.Context, + reason: new Set([ValueReason.Other]), + context: new Set(contextRefOperands), + } + : { + kind: ValueKind.Mutable, + reason: new Set([ValueReason.Other]), + context: new Set(), + }; for (const property of instrValue.properties) { switch (property.kind) { @@ -1593,15 +1597,21 @@ function inferBlock( } case 'LoadLocal': { const lvalue = instr.lvalue; - const effect = - state.isDefined(lvalue) && - state.kind(lvalue).kind === ValueKind.Context - ? Effect.ConditionallyMutate - : Effect.Capture; + CompilerError.invariant( + !( + state.isDefined(lvalue) && + state.kind(lvalue).kind === ValueKind.Context + ), + { + reason: + '[InferReferenceEffects] Unexpected LoadLocal with context kind', + loc: lvalue.loc, + }, + ); state.referenceAndRecordEffects( freezeActions, instrValue.place, - effect, + Effect.Capture, ValueReason.Other, ); lvalue.effect = Effect.ConditionallyMutate; @@ -1932,19 +1942,20 @@ function inferBlock( ); } -function hasContextRefOperand( +function getContextRefOperand( state: InferenceState, instrValue: InstructionValue, -): boolean { +): Array { + const result = []; for (const place of eachInstructionValueOperand(instrValue)) { if ( state.isDefined(place) && state.kind(place).kind === ValueKind.Context ) { - return true; + result.push(place); } } - return false; + return result; } export function getFunctionCallSignature( From deba48a72795d1332fe1df1159fc6b73566667fe Mon Sep 17 00:00:00 2001 From: mofeiZ <34200447+mofeiZ@users.noreply.github.com> Date: Wed, 22 Jan 2025 14:58:52 -0500 Subject: [PATCH 12/19] [compiler] Repro for invalid Array.map type (#32094) See test fixture --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32094). * #32099 * #32104 * #32098 * #32097 * #32096 * #32095 * __->__ #32094 * #32093 --- ...-invalid-mixedreadonly-map-shape.expect.md | 163 ++++++++++++++++++ .../bug-invalid-mixedreadonly-map-shape.js | 56 ++++++ .../packages/snap/src/SproutTodoFilter.ts | 1 + 3 files changed, 220 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.expect.md new file mode 100644 index 0000000000..1418062c33 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.expect.md @@ -0,0 +1,163 @@ + +## Input + +```javascript +import { + arrayPush, + identity, + makeArray, + Stringify, + useFragment, +} from 'shared-runtime'; + +/** + * Bug repro showing why it's invalid for function references to be annotated + * with a `Read` effect when that reference might lead to the function being + * invoked. + * + * Note that currently, `Array.map` is annotated to have `Read` effects on its + * operands. This is incorrect as function effects must be replayed when `map` + * is called + * - Read: non-aliasing data dependency + * - Capture: maybe-aliasing data dependency + * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke + * but only if the value is mutable + * + * Invalid evaluator result: Found differences in evaluator results Non-forget + * (expected): (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2],"count":4}
{"item":1}
+ * Forget: + * (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2,2,2,2],"count":4}
{"item":1}
+ */ + +function Component({extraJsx}) { + const x = makeArray(); + const items = useFragment(); + const jsx = items.a.map((item, i) => { + arrayPush(x, 2); + return ; + }); + const offset = jsx.length; + for (let i = 0; i < extraJsx; i++) { + jsx.push(); + } + const count = jsx.length; + identity(count); + return ( + <> + + {jsx[0]} + + ); +} +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{extraJsx: 0}], + sequentialRenders: [{extraJsx: 0}, {extraJsx: 1}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { + arrayPush, + identity, + makeArray, + Stringify, + useFragment, +} from "shared-runtime"; + +/** + * Bug repro showing why it's invalid for function references to be annotated + * with a `Read` effect when that reference might lead to the function being + * invoked. + * + * Note that currently, `Array.map` is annotated to have `Read` effects on its + * operands. This is incorrect as function effects must be replayed when `map` + * is called + * - Read: non-aliasing data dependency + * - Capture: maybe-aliasing data dependency + * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke + * but only if the value is mutable + * + * Invalid evaluator result: Found differences in evaluator results Non-forget + * (expected): (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2],"count":4}
{"item":1}
+ * Forget: + * (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2,2,2,2],"count":4}
{"item":1}
+ */ + +function Component(t0) { + const $ = _c(9); + const { extraJsx } = t0; + let t1; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t1 = makeArray(); + $[0] = t1; + } else { + t1 = $[0]; + } + const x = t1; + const items = useFragment(); + let jsx; + if ($[1] !== extraJsx || $[2] !== items.a) { + jsx = items.a.map((item, i) => { + arrayPush(x, 2); + return ; + }); + const offset = jsx.length; + for (let i_0 = 0; i_0 < extraJsx; i_0++) { + jsx.push(); + } + $[1] = extraJsx; + $[2] = items.a; + $[3] = jsx; + } else { + jsx = $[3]; + } + + const count = jsx.length; + identity(count); + let t2; + if ($[4] !== count) { + t2 = ; + $[4] = count; + $[5] = t2; + } else { + t2 = $[5]; + } + const t3 = jsx[0]; + let t4; + if ($[6] !== t2 || $[7] !== t3) { + t4 = ( + <> + {t2} + {t3} + + ); + $[6] = t2; + $[7] = t3; + $[8] = t4; + } else { + t4 = $[8]; + } + return t4; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ extraJsx: 0 }], + sequentialRenders: [{ extraJsx: 0 }, { extraJsx: 1 }], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.js new file mode 100644 index 0000000000..d038cf6cd3 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.js @@ -0,0 +1,56 @@ +import { + arrayPush, + identity, + makeArray, + Stringify, + useFragment, +} from 'shared-runtime'; + +/** + * Bug repro showing why it's invalid for function references to be annotated + * with a `Read` effect when that reference might lead to the function being + * invoked. + * + * Note that currently, `Array.map` is annotated to have `Read` effects on its + * operands. This is incorrect as function effects must be replayed when `map` + * is called + * - Read: non-aliasing data dependency + * - Capture: maybe-aliasing data dependency + * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke + * but only if the value is mutable + * + * Invalid evaluator result: Found differences in evaluator results Non-forget + * (expected): (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2],"count":4}
{"item":1}
+ * Forget: + * (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2,2,2,2],"count":4}
{"item":1}
+ */ + +function Component({extraJsx}) { + const x = makeArray(); + const items = useFragment(); + const jsx = items.a.map((item, i) => { + arrayPush(x, 2); + return ; + }); + const offset = jsx.length; + for (let i = 0; i < extraJsx; i++) { + jsx.push(); + } + const count = jsx.length; + identity(count); + return ( + <> + + {jsx[0]} + + ); +} +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{extraJsx: 0}], + sequentialRenders: [{extraJsx: 0}, {extraJsx: 1}], +}; diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index 3610707396..f363fd922e 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -486,6 +486,7 @@ const skipFilter = new Set([ 'bug-aliased-capture-mutate', 'bug-functiondecl-hoisting', 'bug-try-catch-maybe-null-dependency', + 'bug-invalid-mixedreadonly-map-shape', 'bug-type-inference-control-flow', 'reduce-reactive-deps/bug-infer-function-cond-access-not-hoisted', 'bug-invalid-phi-as-dependency', From b83090fca2d96283a5c6153abb65eaa5cc81c9ba Mon Sep 17 00:00:00 2001 From: mofeiZ <34200447+mofeiZ@users.noreply.github.com> Date: Wed, 22 Jan 2025 15:02:51 -0500 Subject: [PATCH 13/19] [compiler] Fix invalid Array.map type (#32095) See test fixture --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32095). * #32099 * #32104 * #32098 * #32097 * #32096 * __->__ #32095 * #32094 * #32093 --- .../src/HIR/ObjectShape.ts | 10 +- .../mixedreadonly-mutating-map.expect.md | 156 ++++++++++++++++++ .../compiler/mixedreadonly-mutating-map.js | 59 +++++++ 3 files changed, 224 insertions(+), 1 deletion(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts index 7c05c05531..f396ae18b0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts @@ -549,8 +549,16 @@ addObject(BUILTIN_SHAPES, BuiltInMixedReadonlyId, [ [ 'map', addFunction(BUILTIN_SHAPES, [], { + /** + * Note `map`'s arguments are annotated as Effect.ConditionallyMutate as + * calling `.map(fn)` might invoke `fn`, which means replaying its + * effects. + * + * (Note that Effect.Read / Effect.Capture on a function type means + * potential data dependency or aliasing respectively.) + */ positionalParams: [], - restParam: Effect.Read, + restParam: Effect.ConditionallyMutate, returnType: {kind: 'Object', shapeId: BuiltInArrayId}, calleeEffect: Effect.ConditionallyMutate, returnValueKind: ValueKind.Mutable, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.expect.md new file mode 100644 index 0000000000..4867388a86 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.expect.md @@ -0,0 +1,156 @@ + +## Input + +```javascript +import { + arrayPush, + identity, + makeArray, + Stringify, + useFragment, +} from 'shared-runtime'; + +/** + * Bug repro showing why it's invalid for function references to be annotated + * with a `Read` effect when that reference might lead to the function being + * invoked. + * + * Note that currently, `Array.map` is annotated to have `Read` effects on its + * operands. This is incorrect as function effects must be replayed when `map` + * is called + * - Read: non-aliasing data dependency + * - Capture: maybe-aliasing data dependency + * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke + * but only if the value is mutable + * + * Invalid evaluator result: Found differences in evaluator results Non-forget + * (expected): (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2],"count":4}
{"item":1}
+ * Forget: + * (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2,2,2,2],"count":4}
{"item":1}
+ */ + +function Component({extraJsx}) { + const x = makeArray(); + const items = useFragment(); + // This closure has the following effects that must be replayed: + // - MaybeFreeze / Capture of `items` + // - ConditionalMutate of x + const jsx = items.a.map((item, i) => { + arrayPush(x, 2); + return ; + }); + const offset = jsx.length; + for (let i = 0; i < extraJsx; i++) { + jsx.push(); + } + const count = jsx.length; + identity(count); + return ( + <> + + {jsx[0]} + + ); +} +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{extraJsx: 0}], + sequentialRenders: [{extraJsx: 0}, {extraJsx: 1}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { + arrayPush, + identity, + makeArray, + Stringify, + useFragment, +} from "shared-runtime"; + +/** + * Bug repro showing why it's invalid for function references to be annotated + * with a `Read` effect when that reference might lead to the function being + * invoked. + * + * Note that currently, `Array.map` is annotated to have `Read` effects on its + * operands. This is incorrect as function effects must be replayed when `map` + * is called + * - Read: non-aliasing data dependency + * - Capture: maybe-aliasing data dependency + * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke + * but only if the value is mutable + * + * Invalid evaluator result: Found differences in evaluator results Non-forget + * (expected): (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2],"count":4}
{"item":1}
+ * Forget: + * (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2,2,2,2],"count":4}
{"item":1}
+ */ + +function Component(t0) { + const $ = _c(6); + const { extraJsx } = t0; + const x = makeArray(); + const items = useFragment(); + + const jsx = items.a.map((item, i) => { + arrayPush(x, 2); + return ; + }); + const offset = jsx.length; + for (let i_0 = 0; i_0 < extraJsx; i_0++) { + jsx.push(); + } + + const count = jsx.length; + identity(count); + let t1; + if ($[0] !== count || $[1] !== x) { + t1 = ; + $[0] = count; + $[1] = x; + $[2] = t1; + } else { + t1 = $[2]; + } + const t2 = jsx[0]; + let t3; + if ($[3] !== t1 || $[4] !== t2) { + t3 = ( + <> + {t1} + {t2} + + ); + $[3] = t1; + $[4] = t2; + $[5] = t3; + } else { + t3 = $[5]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ extraJsx: 0 }], + sequentialRenders: [{ extraJsx: 0 }, { extraJsx: 1 }], +}; + +``` + +### Eval output +(kind: ok)
{"x":[2,2,2],"count":3}
{"item":1}
+
{"x":[2,2,2],"count":4}
{"item":1}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.js new file mode 100644 index 0000000000..858a4ab3dc --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.js @@ -0,0 +1,59 @@ +import { + arrayPush, + identity, + makeArray, + Stringify, + useFragment, +} from 'shared-runtime'; + +/** + * Bug repro showing why it's invalid for function references to be annotated + * with a `Read` effect when that reference might lead to the function being + * invoked. + * + * Note that currently, `Array.map` is annotated to have `Read` effects on its + * operands. This is incorrect as function effects must be replayed when `map` + * is called + * - Read: non-aliasing data dependency + * - Capture: maybe-aliasing data dependency + * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke + * but only if the value is mutable + * + * Invalid evaluator result: Found differences in evaluator results Non-forget + * (expected): (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2],"count":4}
{"item":1}
+ * Forget: + * (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2,2,2,2],"count":4}
{"item":1}
+ */ + +function Component({extraJsx}) { + const x = makeArray(); + const items = useFragment(); + // This closure has the following effects that must be replayed: + // - MaybeFreeze / Capture of `items` + // - ConditionalMutate of x + const jsx = items.a.map((item, i) => { + arrayPush(x, 2); + return ; + }); + const offset = jsx.length; + for (let i = 0; i < extraJsx; i++) { + jsx.push(); + } + const count = jsx.length; + identity(count); + return ( + <> + + {jsx[0]} + + ); +} +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{extraJsx: 0}], + sequentialRenders: [{extraJsx: 0}, {extraJsx: 1}], +}; From a0b91fbd650f9398cca12fcda0c426c434eeb6d8 Mon Sep 17 00:00:00 2001 From: mofeiZ <34200447+mofeiZ@users.noreply.github.com> Date: Wed, 22 Jan 2025 16:21:53 -0500 Subject: [PATCH 14/19] [compiler][ez] Fix main (bad rebase / amend for #32095) (#32160) See title: this fixes test cases broken by https://github.com/facebook/react/pull/32095 adding instead of moving new test fixtures --- .../src/HIR/ObjectShape.ts | 4 +- ...-invalid-mixedreadonly-map-shape.expect.md | 163 ------------------ .../bug-invalid-mixedreadonly-map-shape.js | 56 ------ 3 files changed, 2 insertions(+), 221 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts index f396ae18b0..f51768c586 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts @@ -569,7 +569,7 @@ addObject(BUILTIN_SHAPES, BuiltInMixedReadonlyId, [ 'flatMap', addFunction(BUILTIN_SHAPES, [], { positionalParams: [], - restParam: Effect.Read, + restParam: Effect.ConditionallyMutate, returnType: {kind: 'Object', shapeId: BuiltInArrayId}, calleeEffect: Effect.ConditionallyMutate, returnValueKind: ValueKind.Mutable, @@ -580,7 +580,7 @@ addObject(BUILTIN_SHAPES, BuiltInMixedReadonlyId, [ 'filter', addFunction(BUILTIN_SHAPES, [], { positionalParams: [], - restParam: Effect.Read, + restParam: Effect.ConditionallyMutate, returnType: {kind: 'Object', shapeId: BuiltInArrayId}, calleeEffect: Effect.ConditionallyMutate, returnValueKind: ValueKind.Mutable, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.expect.md deleted file mode 100644 index 1418062c33..0000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.expect.md +++ /dev/null @@ -1,163 +0,0 @@ - -## Input - -```javascript -import { - arrayPush, - identity, - makeArray, - Stringify, - useFragment, -} from 'shared-runtime'; - -/** - * Bug repro showing why it's invalid for function references to be annotated - * with a `Read` effect when that reference might lead to the function being - * invoked. - * - * Note that currently, `Array.map` is annotated to have `Read` effects on its - * operands. This is incorrect as function effects must be replayed when `map` - * is called - * - Read: non-aliasing data dependency - * - Capture: maybe-aliasing data dependency - * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke - * but only if the value is mutable - * - * Invalid evaluator result: Found differences in evaluator results Non-forget - * (expected): (kind: ok) - *
{"x":[2,2,2],"count":3}
{"item":1}
- *
{"x":[2,2,2],"count":4}
{"item":1}
- * Forget: - * (kind: ok) - *
{"x":[2,2,2],"count":3}
{"item":1}
- *
{"x":[2,2,2,2,2,2],"count":4}
{"item":1}
- */ - -function Component({extraJsx}) { - const x = makeArray(); - const items = useFragment(); - const jsx = items.a.map((item, i) => { - arrayPush(x, 2); - return ; - }); - const offset = jsx.length; - for (let i = 0; i < extraJsx; i++) { - jsx.push(); - } - const count = jsx.length; - identity(count); - return ( - <> - - {jsx[0]} - - ); -} -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{extraJsx: 0}], - sequentialRenders: [{extraJsx: 0}, {extraJsx: 1}], -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; -import { - arrayPush, - identity, - makeArray, - Stringify, - useFragment, -} from "shared-runtime"; - -/** - * Bug repro showing why it's invalid for function references to be annotated - * with a `Read` effect when that reference might lead to the function being - * invoked. - * - * Note that currently, `Array.map` is annotated to have `Read` effects on its - * operands. This is incorrect as function effects must be replayed when `map` - * is called - * - Read: non-aliasing data dependency - * - Capture: maybe-aliasing data dependency - * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke - * but only if the value is mutable - * - * Invalid evaluator result: Found differences in evaluator results Non-forget - * (expected): (kind: ok) - *
{"x":[2,2,2],"count":3}
{"item":1}
- *
{"x":[2,2,2],"count":4}
{"item":1}
- * Forget: - * (kind: ok) - *
{"x":[2,2,2],"count":3}
{"item":1}
- *
{"x":[2,2,2,2,2,2],"count":4}
{"item":1}
- */ - -function Component(t0) { - const $ = _c(9); - const { extraJsx } = t0; - let t1; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t1 = makeArray(); - $[0] = t1; - } else { - t1 = $[0]; - } - const x = t1; - const items = useFragment(); - let jsx; - if ($[1] !== extraJsx || $[2] !== items.a) { - jsx = items.a.map((item, i) => { - arrayPush(x, 2); - return ; - }); - const offset = jsx.length; - for (let i_0 = 0; i_0 < extraJsx; i_0++) { - jsx.push(); - } - $[1] = extraJsx; - $[2] = items.a; - $[3] = jsx; - } else { - jsx = $[3]; - } - - const count = jsx.length; - identity(count); - let t2; - if ($[4] !== count) { - t2 = ; - $[4] = count; - $[5] = t2; - } else { - t2 = $[5]; - } - const t3 = jsx[0]; - let t4; - if ($[6] !== t2 || $[7] !== t3) { - t4 = ( - <> - {t2} - {t3} - - ); - $[6] = t2; - $[7] = t3; - $[8] = t4; - } else { - t4 = $[8]; - } - return t4; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{ extraJsx: 0 }], - sequentialRenders: [{ extraJsx: 0 }, { extraJsx: 1 }], -}; - -``` - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.js deleted file mode 100644 index d038cf6cd3..0000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-mixedreadonly-map-shape.js +++ /dev/null @@ -1,56 +0,0 @@ -import { - arrayPush, - identity, - makeArray, - Stringify, - useFragment, -} from 'shared-runtime'; - -/** - * Bug repro showing why it's invalid for function references to be annotated - * with a `Read` effect when that reference might lead to the function being - * invoked. - * - * Note that currently, `Array.map` is annotated to have `Read` effects on its - * operands. This is incorrect as function effects must be replayed when `map` - * is called - * - Read: non-aliasing data dependency - * - Capture: maybe-aliasing data dependency - * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke - * but only if the value is mutable - * - * Invalid evaluator result: Found differences in evaluator results Non-forget - * (expected): (kind: ok) - *
{"x":[2,2,2],"count":3}
{"item":1}
- *
{"x":[2,2,2],"count":4}
{"item":1}
- * Forget: - * (kind: ok) - *
{"x":[2,2,2],"count":3}
{"item":1}
- *
{"x":[2,2,2,2,2,2],"count":4}
{"item":1}
- */ - -function Component({extraJsx}) { - const x = makeArray(); - const items = useFragment(); - const jsx = items.a.map((item, i) => { - arrayPush(x, 2); - return ; - }); - const offset = jsx.length; - for (let i = 0; i < extraJsx; i++) { - jsx.push(); - } - const count = jsx.length; - identity(count); - return ( - <> - - {jsx[0]} - - ); -} -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{extraJsx: 0}], - sequentialRenders: [{extraJsx: 0}, {extraJsx: 1}], -}; From ae9017ceabb2a36a04c249ad5342e0b1af3e1a54 Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Wed, 22 Jan 2025 17:09:41 -0500 Subject: [PATCH 15/19] Move effect dep inference tests to infer-effect-dependencies directory (#32161) Summary: Grouping them to make it easy to see that they are all related Test Plan: --- .../infer-deps-custom-config.expect.md | 0 .../{ => infer-effect-dependencies}/infer-deps-custom-config.js | 0 .../infer-effect-dependencies.expect.md | 0 .../{ => infer-effect-dependencies}/infer-effect-dependencies.js | 0 .../{ => infer-effect-dependencies}/nonreactive-dep.expect.md | 0 .../compiler/{ => infer-effect-dependencies}/nonreactive-dep.js | 0 .../nonreactive-ref-helper.expect.md | 0 .../{ => infer-effect-dependencies}/nonreactive-ref-helper.js | 0 .../{ => infer-effect-dependencies}/nonreactive-ref.expect.md | 0 .../compiler/{ => infer-effect-dependencies}/nonreactive-ref.js | 0 .../{ => infer-effect-dependencies}/outlined-function.expect.md | 0 .../compiler/{ => infer-effect-dependencies}/outlined-function.js | 0 .../pruned-nonreactive-obj.expect.md | 0 .../{ => infer-effect-dependencies}/pruned-nonreactive-obj.js | 0 .../reactive-memberexpr-merge.expect.md | 0 .../{ => infer-effect-dependencies}/reactive-memberexpr-merge.js | 0 .../{ => infer-effect-dependencies}/reactive-memberexpr.expect.md | 0 .../{ => infer-effect-dependencies}/reactive-memberexpr.js | 0 .../reactive-optional-chain.expect.md | 0 .../{ => infer-effect-dependencies}/reactive-optional-chain.js | 0 .../{ => infer-effect-dependencies}/reactive-variable.expect.md | 0 .../compiler/{ => infer-effect-dependencies}/reactive-variable.js | 0 .../todo-import-namespace-useEffect.expect.md | 0 .../todo-import-namespace-useEffect.js | 0 24 files changed, 0 insertions(+), 0 deletions(-) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/infer-deps-custom-config.expect.md (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/infer-deps-custom-config.js (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/infer-effect-dependencies.expect.md (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/infer-effect-dependencies.js (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/nonreactive-dep.expect.md (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/nonreactive-dep.js (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/nonreactive-ref-helper.expect.md (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/nonreactive-ref-helper.js (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/nonreactive-ref.expect.md (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/nonreactive-ref.js (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/outlined-function.expect.md (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/outlined-function.js (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/pruned-nonreactive-obj.expect.md (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/pruned-nonreactive-obj.js (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/reactive-memberexpr-merge.expect.md (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/reactive-memberexpr-merge.js (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/reactive-memberexpr.expect.md (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/reactive-memberexpr.js (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/reactive-optional-chain.expect.md (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/reactive-optional-chain.js (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/reactive-variable.expect.md (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/reactive-variable.js (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/todo-import-namespace-useEffect.expect.md (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{ => infer-effect-dependencies}/todo-import-namespace-useEffect.js (100%) diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-deps-custom-config.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/infer-deps-custom-config.expect.md similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-deps-custom-config.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/infer-deps-custom-config.expect.md diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-deps-custom-config.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/infer-deps-custom-config.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-deps-custom-config.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/infer-deps-custom-config.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/infer-effect-dependencies.expect.md similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/infer-effect-dependencies.expect.md diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/infer-effect-dependencies.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/infer-effect-dependencies.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-dep.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/nonreactive-dep.expect.md similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-dep.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/nonreactive-dep.expect.md diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-dep.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/nonreactive-dep.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-dep.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/nonreactive-dep.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-ref-helper.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/nonreactive-ref-helper.expect.md similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-ref-helper.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/nonreactive-ref-helper.expect.md diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-ref-helper.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/nonreactive-ref-helper.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-ref-helper.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/nonreactive-ref-helper.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-ref.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/nonreactive-ref.expect.md similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-ref.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/nonreactive-ref.expect.md diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-ref.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/nonreactive-ref.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-ref.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/nonreactive-ref.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/outlined-function.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/outlined-function.expect.md similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/outlined-function.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/outlined-function.expect.md diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/outlined-function.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/outlined-function.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/outlined-function.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/outlined-function.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/pruned-nonreactive-obj.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/pruned-nonreactive-obj.expect.md similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/pruned-nonreactive-obj.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/pruned-nonreactive-obj.expect.md diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/pruned-nonreactive-obj.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/pruned-nonreactive-obj.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/pruned-nonreactive-obj.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/pruned-nonreactive-obj.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-memberexpr-merge.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-memberexpr-merge.expect.md similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-memberexpr-merge.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-memberexpr-merge.expect.md diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-memberexpr-merge.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-memberexpr-merge.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-memberexpr-merge.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-memberexpr-merge.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-memberexpr.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-memberexpr.expect.md similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-memberexpr.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-memberexpr.expect.md diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-memberexpr.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-memberexpr.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-memberexpr.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-memberexpr.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-optional-chain.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-optional-chain.expect.md similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-optional-chain.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-optional-chain.expect.md diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-optional-chain.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-optional-chain.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-optional-chain.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-optional-chain.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-variable.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-variable.expect.md similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-variable.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-variable.expect.md diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-variable.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-variable.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-variable.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-variable.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo-import-namespace-useEffect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/todo-import-namespace-useEffect.expect.md similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo-import-namespace-useEffect.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/todo-import-namespace-useEffect.expect.md diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo-import-namespace-useEffect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/todo-import-namespace-useEffect.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo-import-namespace-useEffect.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/todo-import-namespace-useEffect.js From fa5ac23cd0687bac2a081eda5835134e8ac47cb8 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Thu, 23 Jan 2025 15:22:44 -0500 Subject: [PATCH 16/19] [crud] Narrow resource type Small refactor to the `resource` type to narrow it to an arbitrary object or void/null instead of the top type. This makes the overload on useEffect simpler since the return type of create is no longer widened to the top type when we merge their definitions. --- .../react-debug-tools/src/ReactDebugHooks.js | 6 +- .../src/ReactFiberCallUserSpace.js | 2 +- .../src/ReactFiberCommitEffects.js | 8 +- .../react-reconciler/src/ReactFiberHooks.js | 78 +++++++++---------- .../src/ReactInternalTypes.js | 6 +- packages/react/src/ReactHooks.js | 6 +- 6 files changed, 54 insertions(+), 52 deletions(-) diff --git a/packages/react-debug-tools/src/ReactDebugHooks.js b/packages/react-debug-tools/src/ReactDebugHooks.js index f30489b7f6..9c310723b2 100644 --- a/packages/react-debug-tools/src/ReactDebugHooks.js +++ b/packages/react-debug-tools/src/ReactDebugHooks.js @@ -739,11 +739,11 @@ function useHostTransitionStatus(): TransitionStatus { } function useResourceEffect( - create: () => mixed, + create: () => {...} | void | null, createDeps: Array | void | null, - update: ((resource: mixed) => void) | void, + update: ((resource: {...} | void | null) => void) | void, updateDeps: Array | void | null, - destroy: ((resource: mixed) => void) | void, + destroy: ((resource: {...} | void | null) => void) | void, ) { nextHook(); hookLog.push({ diff --git a/packages/react-reconciler/src/ReactFiberCallUserSpace.js b/packages/react-reconciler/src/ReactFiberCallUserSpace.js index a1b86c7560..dd7beecd93 100644 --- a/packages/react-reconciler/src/ReactFiberCallUserSpace.js +++ b/packages/react-reconciler/src/ReactFiberCallUserSpace.js @@ -183,7 +183,7 @@ export const callComponentWillUnmountInDEV: ( const callCreate = { 'react-stack-bottom-frame': function ( effect: Effect, - ): (() => void) | mixed | void { + ): (() => void) | {...} | void | null { if (!enableUseResourceEffectHook) { if (effect.resourceKind != null) { if (__DEV__) { diff --git a/packages/react-reconciler/src/ReactFiberCommitEffects.js b/packages/react-reconciler/src/ReactFiberCommitEffects.js index 6873bdf9e7..a8e08721ee 100644 --- a/packages/react-reconciler/src/ReactFiberCommitEffects.js +++ b/packages/react-reconciler/src/ReactFiberCommitEffects.js @@ -274,6 +274,7 @@ export function commitHookEffectListMount( addendum = ' You returned null. If your effect does not require clean ' + 'up, return undefined (or nothing).'; + // $FlowFixMe (@poteto) this check is safe on arbitrary non-null/void objects } else if (typeof destroy.then === 'function') { addendum = '\n\nIt looks like you wrote ' + @@ -1036,10 +1037,10 @@ function safelyCallDestroy( function safelyCallDestroyWithResource( current: Fiber, nearestMountedAncestor: Fiber | null, - destroy: mixed => void, - resource: mixed, + destroy: ({...}) => void, + resource: {...}, ) { - const destroy_ = resource == null ? destroy : destroy.bind(null, resource); + const destroy_ = destroy.bind(null, resource); if (__DEV__) { runWithFiberInDEV( current, @@ -1050,6 +1051,7 @@ function safelyCallDestroyWithResource( ); } else { try { + // $FlowFixMe(incompatible-call) Already bound to resource destroy_(); } catch (error) { captureCommitPhaseError(current, nearestMountedAncestor, error); diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index e76b78b2ce..5614a8579d 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -205,8 +205,8 @@ export type Hook = { // the additional memory and we can follow up with performance // optimizations later. type EffectInstance = { - resource: mixed, - destroy: void | (() => void) | ((resource: mixed) => void), + resource: {...} | void | null, + destroy: void | (() => void) | ((resource: {...} | void | null) => void), }; export const ResourceEffectIdentityKind: 0 = 0; @@ -229,7 +229,7 @@ export type ResourceEffectIdentity = { resourceKind: typeof ResourceEffectIdentityKind, tag: HookFlags, inst: EffectInstance, - create: () => mixed, + create: () => {...} | void | null, deps: Array | void | null, next: Effect, }; @@ -237,7 +237,7 @@ export type ResourceEffectUpdate = { resourceKind: typeof ResourceEffectUpdateKind, tag: HookFlags, inst: EffectInstance, - update: ((resource: mixed) => void) | void, + update: ((resource: {...} | void | null) => void) | void, deps: Array | void | null, next: Effect, identity: ResourceEffectIdentity, @@ -2540,9 +2540,9 @@ function pushResourceEffect( identityTag: HookFlags, updateTag: HookFlags, inst: EffectInstance, - create: () => mixed, + create: () => {...} | void | null, createDeps: Array | void | null, - update: ((resource: mixed) => void) | void, + update: ((resource: {...} | void | null) => void) | void, updateDeps: Array | void | null, ): Effect { const effectIdentity: ResourceEffectIdentity = { @@ -2694,11 +2694,11 @@ function updateEffect( } function mountResourceEffect( - create: () => mixed, + create: () => {...} | void | null, createDeps: Array | void | null, - update: ((resource: mixed) => void) | void, + update: ((resource: {...} | void | null) => void) | void, updateDeps: Array | void | null, - destroy: ((resource: mixed) => void) | void, + destroy: ((resource: {...} | void | null) => void) | void, ) { if ( __DEV__ && @@ -2730,11 +2730,11 @@ function mountResourceEffect( function mountResourceEffectImpl( fiberFlags: Flags, hookFlags: HookFlags, - create: () => mixed, + create: () => {...} | void | null, createDeps: Array | void | null, - update: ((resource: mixed) => void) | void, + update: ((resource: {...} | void | null) => void) | void, updateDeps: Array | void | null, - destroy: ((resource: mixed) => void) | void, + destroy: ((resource: {...} | void | null) => void) | void, ) { const hook = mountWorkInProgressHook(); currentlyRenderingFiber.flags |= fiberFlags; @@ -2752,11 +2752,11 @@ function mountResourceEffectImpl( } function updateResourceEffect( - create: () => mixed, + create: () => {...} | void | null, createDeps: Array | void | null, - update: ((resource: mixed) => void) | void, + update: ((resource: {...} | void | null) => void) | void, updateDeps: Array | void | null, - destroy: ((resource: mixed) => void) | void, + destroy: ((resource: {...} | void | null) => void) | void, ) { updateResourceEffectImpl( PassiveEffect, @@ -2772,11 +2772,11 @@ function updateResourceEffect( function updateResourceEffectImpl( fiberFlags: Flags, hookFlags: HookFlags, - create: () => mixed, + create: () => {...} | void | null, createDeps: Array | void | null, - update: ((resource: mixed) => void) | void, + update: ((resource: {...} | void | null) => void) | void, updateDeps: Array | void | null, - destroy: ((resource: mixed) => void) | void, + destroy: ((resource: {...} | void | null) => void) | void, ) { const hook = updateWorkInProgressHook(); const effect: Effect = hook.memoizedState; @@ -4245,11 +4245,11 @@ if (__DEV__) { if (enableUseResourceEffectHook) { (HooksDispatcherOnMountInDEV: Dispatcher).useResourceEffect = function useResourceEffect( - create: () => mixed, + create: () => {...} | void | null, createDeps: Array | void | null, - update: ((resource: mixed) => void) | void, + update: ((resource: {...} | void | null) => void) | void, updateDeps: Array | void | null, - destroy: ((resource: mixed) => void) | void, + destroy: ((resource: {...} | void | null) => void) | void, ): void { currentHookNameInDev = 'useResourceEffect'; mountHookTypesDev(); @@ -4433,11 +4433,11 @@ if (__DEV__) { if (enableUseResourceEffectHook) { (HooksDispatcherOnMountWithHookTypesInDEV: Dispatcher).useResourceEffect = function useResourceEffect( - create: () => mixed, + create: () => {...} | void | null, createDeps: Array | void | null, - update: ((resource: mixed) => void) | void, + update: ((resource: {...} | void | null) => void) | void, updateDeps: Array | void | null, - destroy: ((resource: mixed) => void) | void, + destroy: ((resource: {...} | void | null) => void) | void, ): void { currentHookNameInDev = 'useResourceEffect'; updateHookTypesDev(); @@ -4620,11 +4620,11 @@ if (__DEV__) { if (enableUseResourceEffectHook) { (HooksDispatcherOnUpdateInDEV: Dispatcher).useResourceEffect = function useResourceEffect( - create: () => mixed, + create: () => {...} | void | null, createDeps: Array | void | null, - update: ((resource: mixed) => void) | void, + update: ((resource: {...} | void | null) => void) | void, updateDeps: Array | void | null, - destroy: ((resource: mixed) => void) | void, + destroy: ((resource: {...} | void | null) => void) | void, ) { currentHookNameInDev = 'useResourceEffect'; updateHookTypesDev(); @@ -4807,11 +4807,11 @@ if (__DEV__) { if (enableUseResourceEffectHook) { (HooksDispatcherOnRerenderInDEV: Dispatcher).useResourceEffect = function useResourceEffect( - create: () => mixed, + create: () => {...} | void | null, createDeps: Array | void | null, - update: ((resource: mixed) => void) | void, + update: ((resource: {...} | void | null) => void) | void, updateDeps: Array | void | null, - destroy: ((resource: mixed) => void) | void, + destroy: ((resource: {...} | void | null) => void) | void, ) { currentHookNameInDev = 'useResourceEffect'; updateHookTypesDev(); @@ -5019,11 +5019,11 @@ if (__DEV__) { if (enableUseResourceEffectHook) { (InvalidNestedHooksDispatcherOnMountInDEV: Dispatcher).useResourceEffect = function useResourceEffect( - create: () => mixed, + create: () => {...} | void | null, createDeps: Array | void | null, - update: ((resource: mixed) => void) | void, + update: ((resource: {...} | void | null) => void) | void, updateDeps: Array | void | null, - destroy: ((resource: mixed) => void) | void, + destroy: ((resource: {...} | void | null) => void) | void, ): void { currentHookNameInDev = 'useResourceEffect'; warnInvalidHookAccess(); @@ -5232,11 +5232,11 @@ if (__DEV__) { if (enableUseResourceEffectHook) { (InvalidNestedHooksDispatcherOnUpdateInDEV: Dispatcher).useResourceEffect = function useResourceEffect( - create: () => mixed, + create: () => {...} | void | null, createDeps: Array | void | null, - update: ((resource: mixed) => void) | void, + update: ((resource: {...} | void | null) => void) | void, updateDeps: Array | void | null, - destroy: ((resource: mixed) => void) | void, + destroy: ((resource: {...} | void | null) => void) | void, ) { currentHookNameInDev = 'useResourceEffect'; warnInvalidHookAccess(); @@ -5445,11 +5445,11 @@ if (__DEV__) { if (enableUseResourceEffectHook) { (InvalidNestedHooksDispatcherOnRerenderInDEV: Dispatcher).useResourceEffect = function useResourceEffect( - create: () => mixed, + create: () => {...} | void | null, createDeps: Array | void | null, - update: ((resource: mixed) => void) | void, + update: ((resource: {...} | void | null) => void) | void, updateDeps: Array | void | null, - destroy: ((resource: mixed) => void) | void, + destroy: ((resource: {...} | void | null) => void) | void, ) { currentHookNameInDev = 'useResourceEffect'; warnInvalidHookAccess(); diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index 0c5504cab4..e31b7cefd1 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -398,11 +398,11 @@ export type Dispatcher = { useEffectEvent?: ) => mixed>(callback: F) => F, // TODO: Non-nullable once `enableUseResourceEffectHook` is on everywhere. useResourceEffect?: ( - create: () => mixed, + create: () => {...} | void | null, createDeps: Array | void | null, - update: ((resource: mixed) => void) | void, + update: ((resource: {...} | void | null) => void) | void, updateDeps: Array | void | null, - destroy: ((resource: mixed) => void) | void, + destroy: ((resource: {...} | void | null) => void) | void, ) => void, useInsertionEffect( create: () => (() => void) | void, diff --git a/packages/react/src/ReactHooks.js b/packages/react/src/ReactHooks.js index 32f4926888..ff45b5415c 100644 --- a/packages/react/src/ReactHooks.js +++ b/packages/react/src/ReactHooks.js @@ -202,11 +202,11 @@ export function useEffectEvent) => mixed>( } export function useResourceEffect( - create: () => mixed, + create: () => {...} | void | null, createDeps: Array | void | null, - update: ((resource: mixed) => void) | void, + update: ((resource: {...} | void | null) => void) | void, updateDeps: Array | void | null, - destroy: ((resource: mixed) => void) | void, + destroy: ((resource: {...} | void | null) => void) | void, ): void { if (!enableUseResourceEffectHook) { throw new Error('Not implemented.'); From fda22f22defe373977a92051387d61850c322144 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Thu, 23 Jan 2025 16:32:03 -0500 Subject: [PATCH 17/19] [crud] Rename useResourceEffect flag Rename the flag in preparation for the overload. --- .../src/ReactFiberCallUserSpace.js | 4 ++-- .../src/ReactFiberCommitEffects.js | 14 +++++------ .../react-reconciler/src/ReactFiberHooks.js | 24 +++++++++---------- packages/react-server/src/ReactFizzHooks.js | 4 ++-- packages/react/src/ReactClient.js | 4 ++-- packages/react/src/ReactHooks.js | 4 ++-- packages/shared/ReactFeatureFlags.js | 2 +- 7 files changed, 28 insertions(+), 28 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCallUserSpace.js b/packages/react-reconciler/src/ReactFiberCallUserSpace.js index dd7beecd93..84e470c160 100644 --- a/packages/react-reconciler/src/ReactFiberCallUserSpace.js +++ b/packages/react-reconciler/src/ReactFiberCallUserSpace.js @@ -18,7 +18,7 @@ import { ResourceEffectIdentityKind, ResourceEffectUpdateKind, } from './ReactFiberHooks'; -import {enableUseResourceEffectHook} from 'shared/ReactFeatureFlags'; +import {enableUseEffectCRUDOverload} from 'shared/ReactFeatureFlags'; // These indirections exists so we can exclude its stack frame in DEV (and anything below it). // TODO: Consider marking the whole bundle instead of these boundaries. @@ -184,7 +184,7 @@ const callCreate = { 'react-stack-bottom-frame': function ( effect: Effect, ): (() => void) | {...} | void | null { - if (!enableUseResourceEffectHook) { + if (!enableUseEffectCRUDOverload) { if (effect.resourceKind != null) { if (__DEV__) { console.error( diff --git a/packages/react-reconciler/src/ReactFiberCommitEffects.js b/packages/react-reconciler/src/ReactFiberCommitEffects.js index a8e08721ee..554fa20294 100644 --- a/packages/react-reconciler/src/ReactFiberCommitEffects.js +++ b/packages/react-reconciler/src/ReactFiberCommitEffects.js @@ -22,7 +22,7 @@ import { enableProfilerCommitHooks, enableProfilerNestedUpdatePhase, enableSchedulingProfiler, - enableUseResourceEffectHook, + enableUseEffectCRUDOverload, enableViewTransition, } from 'shared/ReactFeatureFlags'; import { @@ -160,7 +160,7 @@ export function commitHookEffectListMount( // Mount let destroy; - if (enableUseResourceEffectHook) { + if (enableUseEffectCRUDOverload) { if (effect.resourceKind === ResourceEffectIdentityKind) { if (__DEV__) { effect.inst.resource = runWithFiberInDEV( @@ -200,7 +200,7 @@ export function commitHookEffectListMount( if ((flags & HookInsertion) !== NoHookEffect) { setIsRunningInsertionEffect(true); } - if (enableUseResourceEffectHook) { + if (enableUseEffectCRUDOverload) { if (effect.resourceKind == null) { destroy = runWithFiberInDEV( finishedWork, @@ -219,7 +219,7 @@ export function commitHookEffectListMount( setIsRunningInsertionEffect(false); } } else { - if (enableUseResourceEffectHook) { + if (enableUseEffectCRUDOverload) { if (effect.resourceKind == null) { const create = effect.create; const inst = effect.inst; @@ -262,7 +262,7 @@ export function commitHookEffectListMount( } else if ((effect.tag & HookInsertion) !== NoFlags) { hookName = 'useInsertionEffect'; } else if ( - enableUseResourceEffectHook && + enableUseEffectCRUDOverload && effect.resourceKind != null ) { hookName = 'useResourceEffect'; @@ -338,7 +338,7 @@ export function commitHookEffectListUnmount( const inst = effect.inst; const destroy = inst.destroy; if (destroy !== undefined) { - if (enableUseResourceEffectHook) { + if (enableUseEffectCRUDOverload) { if (effect.resourceKind == null) { inst.destroy = undefined; } @@ -358,7 +358,7 @@ export function commitHookEffectListUnmount( setIsRunningInsertionEffect(true); } } - if (enableUseResourceEffectHook) { + if (enableUseEffectCRUDOverload) { if ( effect.resourceKind === ResourceEffectIdentityKind && effect.inst.resource != null diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 5614a8579d..229b5a3c9a 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -38,7 +38,7 @@ import { enableSchedulingProfiler, enableTransitionTracing, enableUseEffectEventHook, - enableUseResourceEffectHook, + enableUseEffectCRUDOverload, enableLegacyCache, disableLegacyMode, enableNoCloningMemoCache, @@ -3938,7 +3938,7 @@ export const ContextOnlyDispatcher: Dispatcher = { if (enableUseEffectEventHook) { (ContextOnlyDispatcher: Dispatcher).useEffectEvent = throwInvalidHookError; } -if (enableUseResourceEffectHook) { +if (enableUseEffectCRUDOverload) { (ContextOnlyDispatcher: Dispatcher).useResourceEffect = throwInvalidHookError; } @@ -3971,7 +3971,7 @@ const HooksDispatcherOnMount: Dispatcher = { if (enableUseEffectEventHook) { (HooksDispatcherOnMount: Dispatcher).useEffectEvent = mountEvent; } -if (enableUseResourceEffectHook) { +if (enableUseEffectCRUDOverload) { (HooksDispatcherOnMount: Dispatcher).useResourceEffect = mountResourceEffect; } @@ -4004,7 +4004,7 @@ const HooksDispatcherOnUpdate: Dispatcher = { if (enableUseEffectEventHook) { (HooksDispatcherOnUpdate: Dispatcher).useEffectEvent = updateEvent; } -if (enableUseResourceEffectHook) { +if (enableUseEffectCRUDOverload) { (HooksDispatcherOnUpdate: Dispatcher).useResourceEffect = updateResourceEffect; } @@ -4038,7 +4038,7 @@ const HooksDispatcherOnRerender: Dispatcher = { if (enableUseEffectEventHook) { (HooksDispatcherOnRerender: Dispatcher).useEffectEvent = updateEvent; } -if (enableUseResourceEffectHook) { +if (enableUseEffectCRUDOverload) { (HooksDispatcherOnRerender: Dispatcher).useResourceEffect = updateResourceEffect; } @@ -4242,7 +4242,7 @@ if (__DEV__) { return mountEvent(callback); }; } - if (enableUseResourceEffectHook) { + if (enableUseEffectCRUDOverload) { (HooksDispatcherOnMountInDEV: Dispatcher).useResourceEffect = function useResourceEffect( create: () => {...} | void | null, @@ -4430,7 +4430,7 @@ if (__DEV__) { return mountEvent(callback); }; } - if (enableUseResourceEffectHook) { + if (enableUseEffectCRUDOverload) { (HooksDispatcherOnMountWithHookTypesInDEV: Dispatcher).useResourceEffect = function useResourceEffect( create: () => {...} | void | null, @@ -4617,7 +4617,7 @@ if (__DEV__) { return updateEvent(callback); }; } - if (enableUseResourceEffectHook) { + if (enableUseEffectCRUDOverload) { (HooksDispatcherOnUpdateInDEV: Dispatcher).useResourceEffect = function useResourceEffect( create: () => {...} | void | null, @@ -4804,7 +4804,7 @@ if (__DEV__) { return updateEvent(callback); }; } - if (enableUseResourceEffectHook) { + if (enableUseEffectCRUDOverload) { (HooksDispatcherOnRerenderInDEV: Dispatcher).useResourceEffect = function useResourceEffect( create: () => {...} | void | null, @@ -5016,7 +5016,7 @@ if (__DEV__) { return mountEvent(callback); }; } - if (enableUseResourceEffectHook) { + if (enableUseEffectCRUDOverload) { (InvalidNestedHooksDispatcherOnMountInDEV: Dispatcher).useResourceEffect = function useResourceEffect( create: () => {...} | void | null, @@ -5229,7 +5229,7 @@ if (__DEV__) { return updateEvent(callback); }; } - if (enableUseResourceEffectHook) { + if (enableUseEffectCRUDOverload) { (InvalidNestedHooksDispatcherOnUpdateInDEV: Dispatcher).useResourceEffect = function useResourceEffect( create: () => {...} | void | null, @@ -5442,7 +5442,7 @@ if (__DEV__) { return updateEvent(callback); }; } - if (enableUseResourceEffectHook) { + if (enableUseEffectCRUDOverload) { (InvalidNestedHooksDispatcherOnRerenderInDEV: Dispatcher).useResourceEffect = function useResourceEffect( create: () => {...} | void | null, diff --git a/packages/react-server/src/ReactFizzHooks.js b/packages/react-server/src/ReactFizzHooks.js index 0db0b00b3b..63e8576ca7 100644 --- a/packages/react-server/src/ReactFizzHooks.js +++ b/packages/react-server/src/ReactFizzHooks.js @@ -40,7 +40,7 @@ import {createFastHash} from './ReactServerStreamConfig'; import { enableUseEffectEventHook, - enableUseResourceEffectHook, + enableUseEffectCRUDOverload, } from 'shared/ReactFeatureFlags'; import is from 'shared/objectIs'; import { @@ -866,7 +866,7 @@ export const HooksDispatcher: Dispatcher = supportsClientAPIs if (enableUseEffectEventHook) { HooksDispatcher.useEffectEvent = useEffectEvent; } -if (enableUseResourceEffectHook) { +if (enableUseEffectCRUDOverload) { HooksDispatcher.useResourceEffect = supportsClientAPIs ? noop : clientHookNotSupported; diff --git a/packages/react/src/ReactClient.js b/packages/react/src/ReactClient.js index f633c7617d..2bacbc8567 100644 --- a/packages/react/src/ReactClient.js +++ b/packages/react/src/ReactClient.js @@ -65,7 +65,7 @@ import {addTransitionType} from './ReactTransitionType'; import {act} from './ReactAct'; import {captureOwnerStack} from './ReactOwnerStack'; import * as ReactCompilerRuntime from './ReactCompilerRuntime'; -import {enableUseResourceEffectHook} from 'shared/ReactFeatureFlags'; +import {enableUseEffectCRUDOverload} from 'shared/ReactFeatureFlags'; const Children = { map, @@ -134,4 +134,4 @@ export { }; export const experimental_useResourceEffect: typeof useResourceEffect | void = - enableUseResourceEffectHook ? useResourceEffect : undefined; + enableUseEffectCRUDOverload ? useResourceEffect : undefined; diff --git a/packages/react/src/ReactHooks.js b/packages/react/src/ReactHooks.js index ff45b5415c..677e0d705b 100644 --- a/packages/react/src/ReactHooks.js +++ b/packages/react/src/ReactHooks.js @@ -18,7 +18,7 @@ import {REACT_CONSUMER_TYPE} from 'shared/ReactSymbols'; import ReactSharedInternals from 'shared/ReactSharedInternals'; -import {enableUseResourceEffectHook} from 'shared/ReactFeatureFlags'; +import {enableUseEffectCRUDOverload} from 'shared/ReactFeatureFlags'; type BasicStateAction = (S => S) | S; type Dispatch = A => void; @@ -208,7 +208,7 @@ export function useResourceEffect( updateDeps: Array | void | null, destroy: ((resource: {...} | void | null) => void) | void, ): void { - if (!enableUseResourceEffectHook) { + if (!enableUseEffectCRUDOverload) { throw new Error('Not implemented.'); } const dispatcher = resolveDispatcher(); diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 6a77c29ff3..afe44bc881 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -155,7 +155,7 @@ export const enableInfiniteRenderLoopDetection = false; /** * Experimental new hook for better managing resources in effects. */ -export const enableUseResourceEffectHook = false; +export const enableUseEffectCRUDOverload = false; // ----------------------------------------------------------------------------- // Ready for next major. From f838bdf0b714d8ea6066677fec131da6b5bfd72a Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Thu, 23 Jan 2025 16:55:13 -0500 Subject: [PATCH 18/19] [crud] Merge useResourceEffect into useEffect Merges the useResourceEffect API into useEffect while keeping the underlying implementation the same. useResourceEffect will be removed in the next diff. To fork between behavior we rely on a `typeof` check for the updater or destroy function in addition to the CRUD feature flag. This does now have to be checked every time (instead of inlined statically like before due to them being different hooks) which will incur some non-zero amount (possibly negligble) of overhead for every effect. --- .../react-debug-tools/src/ReactDebugHooks.js | 7 +- .../ReactDOMServerIntegrationHooks-test.js | 8 +- .../src/ReactFiberCallUserSpace.js | 4 +- .../src/ReactFiberCommitEffects.js | 43 +-- .../react-reconciler/src/ReactFiberHooks.js | 268 ++++++++++++++---- .../src/ReactInternalTypes.js | 7 +- .../ReactHooksWithNoopRenderer-test.js | 90 ++---- packages/react/src/ReactHooks.js | 26 +- .../ReactFeatureFlags.native-fb-dynamic.js | 2 +- .../forks/ReactFeatureFlags.native-fb.js | 2 +- .../forks/ReactFeatureFlags.native-oss.js | 2 +- .../forks/ReactFeatureFlags.test-renderer.js | 2 +- ...actFeatureFlags.test-renderer.native-fb.js | 2 +- .../ReactFeatureFlags.test-renderer.www.js | 2 +- .../forks/ReactFeatureFlags.www-dynamic.js | 2 +- .../shared/forks/ReactFeatureFlags.www.js | 2 +- scripts/error-codes/codes.json | 3 +- 17 files changed, 304 insertions(+), 168 deletions(-) diff --git a/packages/react-debug-tools/src/ReactDebugHooks.js b/packages/react-debug-tools/src/ReactDebugHooks.js index 9c310723b2..f45ccfecdc 100644 --- a/packages/react-debug-tools/src/ReactDebugHooks.js +++ b/packages/react-debug-tools/src/ReactDebugHooks.js @@ -373,8 +373,11 @@ function useInsertionEffect( } function useEffect( - create: () => (() => void) | void, - inputs: Array | void | null, + create: (() => (() => void) | void) | (() => {...} | void | null), + createDeps: Array | void | null, + update?: ((resource: {...} | void | null) => void) | void, + updateDeps?: Array | void | null, + destroy?: ((resource: {...} | void | null) => void) | void, ): void { nextHook(); hookLog.push({ diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js index b79e59ad00..840d6c5b15 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js @@ -27,7 +27,6 @@ let useRef; let useImperativeHandle; let useInsertionEffect; let useLayoutEffect; -let useResourceEffect; let useDebugValue; let forwardRef; let yieldedValues; @@ -52,7 +51,6 @@ function initModules() { useImperativeHandle = React.useImperativeHandle; useInsertionEffect = React.useInsertionEffect; useLayoutEffect = React.useLayoutEffect; - useResourceEffect = React.experimental_useResourceEffect; forwardRef = React.forwardRef; yieldedValues = []; @@ -655,15 +653,15 @@ describe('ReactDOMServerHooks', () => { }); }); - describe('useResourceEffect', () => { + describe('useEffect with CRUD overload', () => { gate(flags => { - if (flags.enableUseResourceEffectHook) { + if (flags.enableUseEffectCRUDOverload) { const yields = []; itRenders( 'should ignore resource effects on the server', async render => { function Counter(props) { - useResourceEffect( + useEffect( () => { yieldValue('created on client'); return {resource_counter: props.count}; diff --git a/packages/react-reconciler/src/ReactFiberCallUserSpace.js b/packages/react-reconciler/src/ReactFiberCallUserSpace.js index 84e470c160..a9b5590f38 100644 --- a/packages/react-reconciler/src/ReactFiberCallUserSpace.js +++ b/packages/react-reconciler/src/ReactFiberCallUserSpace.js @@ -188,7 +188,7 @@ const callCreate = { if (effect.resourceKind != null) { if (__DEV__) { console.error( - 'Expected only SimpleEffects when enableUseResourceEffectHook is disabled, ' + + 'Expected only SimpleEffects when enableUseEffectCRUDOverload is disabled, ' + 'got %s', effect.resourceKind, ); @@ -254,7 +254,7 @@ const callDestroy = { export const callDestroyInDEV: ( current: Fiber, nearestMountedAncestor: Fiber | null, - destroy: () => void, + destroy: (() => void) | (({...}) => void), ) => void = __DEV__ ? // We use this technique to trick minifiers to preserve the function name. (callDestroy['react-stack-bottom-frame'].bind(callDestroy): any) diff --git a/packages/react-reconciler/src/ReactFiberCommitEffects.js b/packages/react-reconciler/src/ReactFiberCommitEffects.js index 554fa20294..05cf17a342 100644 --- a/packages/react-reconciler/src/ReactFiberCommitEffects.js +++ b/packages/react-reconciler/src/ReactFiberCommitEffects.js @@ -170,8 +170,9 @@ export function commitHookEffectListMount( ); if (effect.inst.resource == null) { console.error( - 'useResourceEffect must provide a callback which returns a resource. ' + - 'If a managed resource is not needed here, use useEffect. Received %s', + 'useEffect must provide a callback which returns a resource. ' + + 'If a managed resource is not needed here, do not provide an updater or ' + + 'destroy callback. Received %s', effect.inst.resource, ); } @@ -230,7 +231,7 @@ export function commitHookEffectListMount( if (effect.resourceKind != null) { if (__DEV__) { console.error( - 'Expected only SimpleEffects when enableUseResourceEffectHook is disabled, ' + + 'Expected only SimpleEffects when enableUseEffectCRUDOverload is disabled, ' + 'got %s', effect.resourceKind, ); @@ -261,11 +262,6 @@ export function commitHookEffectListMount( hookName = 'useLayoutEffect'; } else if ((effect.tag & HookInsertion) !== NoFlags) { hookName = 'useInsertionEffect'; - } else if ( - enableUseEffectCRUDOverload && - effect.resourceKind != null - ) { - hookName = 'useResourceEffect'; } else { hookName = 'useEffect'; } @@ -363,7 +359,7 @@ export function commitHookEffectListUnmount( effect.resourceKind === ResourceEffectIdentityKind && effect.inst.resource != null ) { - safelyCallDestroyWithResource( + safelyCallDestroy( finishedWork, nearestMountedAncestor, destroy, @@ -1015,32 +1011,11 @@ export function safelyDetachRef( function safelyCallDestroy( current: Fiber, nearestMountedAncestor: Fiber | null, - destroy: () => void, + destroy: (() => void) | (({...}) => void), + resource?: {...} | void | null, ) { - if (__DEV__) { - runWithFiberInDEV( - current, - callDestroyInDEV, - current, - nearestMountedAncestor, - destroy, - ); - } else { - try { - destroy(); - } catch (error) { - captureCommitPhaseError(current, nearestMountedAncestor, error); - } - } -} - -function safelyCallDestroyWithResource( - current: Fiber, - nearestMountedAncestor: Fiber | null, - destroy: ({...}) => void, - resource: {...}, -) { - const destroy_ = destroy.bind(null, resource); + // $FlowFixMe[extra-arg] @poteto this is safe either way because the extra arg is ignored if it's not a CRUD effect + const destroy_ = resource == null ? destroy : destroy.bind(null, resource); if (__DEV__) { runWithFiberInDEV( current, diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 229b5a3c9a..110f896deb 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -2523,12 +2523,15 @@ function pushSimpleEffect( tag: HookFlags, inst: EffectInstance, create: () => (() => void) | void, - deps: Array | void | null, + createDeps: Array | void | null, + update?: ((resource: {...} | void | null) => void) | void, + updateDeps?: Array | void | null, + destroy?: ((resource: {...} | void | null) => void) | void, ): Effect { const effect: Effect = { tag, create, - deps, + deps: createDeps, inst, // Circular next: (null: any), @@ -2608,10 +2611,13 @@ function mountEffectImpl( fiberFlags: Flags, hookFlags: HookFlags, create: () => (() => void) | void, - deps: Array | void | null, + createDeps: Array | void | null, + update?: ((resource: {...} | void | null) => void) | void, + updateDeps?: Array | void | null, + destroy?: ((resource: {...} | void | null) => void) | void, ): void { const hook = mountWorkInProgressHook(); - const nextDeps = deps === undefined ? null : deps; + const nextDeps = createDeps === undefined ? null : createDeps; currentlyRenderingFiber.flags |= fiberFlags; hook.memoizedState = pushSimpleEffect( HookHasEffect | hookFlags, @@ -2662,35 +2668,89 @@ function updateEffectImpl( } function mountEffect( - create: () => (() => void) | void, - deps: Array | void | null, + create: (() => (() => void) | void) | (() => {...} | void | null), + createDeps: Array | void | null, + update?: ((resource: {...} | void | null) => void) | void, + updateDeps?: Array | void | null, + destroy?: ((resource: {...} | void | null) => void) | void, ): void { if ( __DEV__ && (currentlyRenderingFiber.mode & StrictEffectsMode) !== NoMode && (currentlyRenderingFiber.mode & NoStrictPassiveEffectsMode) === NoMode ) { - mountEffectImpl( - MountPassiveDevEffect | PassiveEffect | PassiveStaticEffect, - HookPassive, - create, - deps, - ); + if ( + enableUseEffectCRUDOverload && + (typeof update === 'function' || typeof destroy === 'function') + ) { + mountResourceEffectImpl( + MountPassiveDevEffect | PassiveEffect | PassiveStaticEffect, + HookPassive, + create, + createDeps, + update, + updateDeps, + destroy, + ); + } else { + mountEffectImpl( + MountPassiveDevEffect | PassiveEffect | PassiveStaticEffect, + HookPassive, + // $FlowFixMe[incompatible-call] @poteto it's not possible to narrow `create` without calling it. + create, + createDeps, + ); + } } else { - mountEffectImpl( - PassiveEffect | PassiveStaticEffect, - HookPassive, - create, - deps, - ); + if ( + enableUseEffectCRUDOverload && + (typeof update === 'function' || typeof destroy === 'function') + ) { + mountResourceEffectImpl( + PassiveEffect | PassiveStaticEffect, + HookPassive, + create, + createDeps, + update, + updateDeps, + destroy, + ); + } else { + mountEffectImpl( + PassiveEffect | PassiveStaticEffect, + HookPassive, + // $FlowFixMe[incompatible-call] @poteto it's not possible to narrow `create` without calling it. + create, + createDeps, + ); + } } } function updateEffect( - create: () => (() => void) | void, - deps: Array | void | null, + create: (() => (() => void) | void) | (() => {...} | void | null), + createDeps: Array | void | null, + update?: ((resource: {...} | void | null) => void) | void, + updateDeps?: Array | void | null, + destroy?: ((resource: {...} | void | null) => void) | void, ): void { - updateEffectImpl(PassiveEffect, HookPassive, create, deps); + if ( + enableUseEffectCRUDOverload && + (typeof update === 'function' || typeof destroy === 'function') + ) { + updateResourceEffectImpl( + PassiveEffect, + HookPassive, + create, + createDeps, + update, + updateDeps, + destroy, + ); + } else { + // $FlowFixMe[incompatible-call] @poteto it's not possible to narrow `create` without calling it. + updateEffectImpl(PassiveEffect, HookPassive, create, createDeps); + } } function mountResourceEffect( @@ -2705,15 +2765,6 @@ function mountResourceEffect( (currentlyRenderingFiber.mode & StrictEffectsMode) !== NoMode && (currentlyRenderingFiber.mode & NoStrictPassiveEffectsMode) === NoMode ) { - mountResourceEffectImpl( - MountPassiveDevEffect | PassiveEffect | PassiveStaticEffect, - HookPassive, - create, - createDeps, - update, - updateDeps, - destroy, - ); } else { mountResourceEffectImpl( PassiveEffect | PassiveStaticEffect, @@ -4087,13 +4138,30 @@ if (__DEV__) { return readContext(context); }, useEffect( - create: () => (() => void) | void, - deps: Array | void | null, + create: (() => (() => void) | void) | (() => {...} | void | null), + createDeps: Array | void | null, + update?: ((resource: {...} | void | null) => void) | void, + updateDeps?: Array | void | null, + destroy?: ((resource: {...} | void | null) => void) | void, ): void { currentHookNameInDev = 'useEffect'; mountHookTypesDev(); - checkDepsAreArrayDev(deps); - return mountEffect(create, deps); + if ( + enableUseEffectCRUDOverload && + (typeof update === 'function' || typeof destroy === 'function') + ) { + checkDepsAreNonEmptyArrayDev(updateDeps); + return mountResourceEffect( + create, + createDeps, + update, + updateDeps, + destroy, + ); + } else { + checkDepsAreArrayDev(createDeps); + return mountEffect(create, createDeps); + } }, useImperativeHandle( ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, @@ -4280,12 +4348,28 @@ if (__DEV__) { return readContext(context); }, useEffect( - create: () => (() => void) | void, - deps: Array | void | null, + create: (() => (() => void) | void) | (() => {...} | void | null), + createDeps: Array | void | null, + update?: ((resource: {...} | void | null) => void) | void, + updateDeps?: Array | void | null, + destroy?: ((resource: {...} | void | null) => void) | void, ): void { currentHookNameInDev = 'useEffect'; updateHookTypesDev(); - return mountEffect(create, deps); + if ( + enableUseEffectCRUDOverload && + (typeof update === 'function' || typeof destroy === 'function') + ) { + return mountResourceEffect( + create, + createDeps, + update, + updateDeps, + destroy, + ); + } else { + return mountEffect(create, createDeps); + } }, useImperativeHandle( ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, @@ -4467,12 +4551,28 @@ if (__DEV__) { return readContext(context); }, useEffect( - create: () => (() => void) | void, - deps: Array | void | null, + create: (() => (() => void) | void) | (() => {...} | void | null), + createDeps: Array | void | null, + update?: ((resource: {...} | void | null) => void) | void, + updateDeps?: Array | void | null, + destroy?: ((resource: {...} | void | null) => void) | void, ): void { currentHookNameInDev = 'useEffect'; updateHookTypesDev(); - return updateEffect(create, deps); + if ( + enableUseEffectCRUDOverload && + (typeof update === 'function' || typeof destroy === 'function') + ) { + return updateResourceEffect( + create, + createDeps, + update, + updateDeps, + destroy, + ); + } else { + return updateEffect(create, createDeps); + } }, useImperativeHandle( ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, @@ -4654,12 +4754,28 @@ if (__DEV__) { return readContext(context); }, useEffect( - create: () => (() => void) | void, - deps: Array | void | null, + create: (() => (() => void) | void) | (() => {...} | void | null), + createDeps: Array | void | null, + update?: ((resource: {...} | void | null) => void) | void, + updateDeps?: Array | void | null, + destroy?: ((resource: {...} | void | null) => void) | void, ): void { currentHookNameInDev = 'useEffect'; updateHookTypesDev(); - return updateEffect(create, deps); + if ( + enableUseEffectCRUDOverload && + (typeof update === 'function' || typeof destroy === 'function') + ) { + return updateResourceEffect( + create, + createDeps, + update, + updateDeps, + destroy, + ); + } else { + return updateEffect(create, createDeps); + } }, useImperativeHandle( ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, @@ -4847,13 +4963,29 @@ if (__DEV__) { return readContext(context); }, useEffect( - create: () => (() => void) | void, - deps: Array | void | null, + create: (() => (() => void) | void) | (() => {...} | void | null), + createDeps: Array | void | null, + update?: ((resource: {...} | void | null) => void) | void, + updateDeps?: Array | void | null, + destroy?: ((resource: {...} | void | null) => void) | void, ): void { currentHookNameInDev = 'useEffect'; warnInvalidHookAccess(); mountHookTypesDev(); - return mountEffect(create, deps); + if ( + enableUseEffectCRUDOverload && + (typeof update === 'function' || typeof destroy === 'function') + ) { + return mountResourceEffect( + create, + createDeps, + update, + updateDeps, + destroy, + ); + } else { + return mountEffect(create, createDeps); + } }, useImperativeHandle( ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, @@ -5060,13 +5192,29 @@ if (__DEV__) { return readContext(context); }, useEffect( - create: () => (() => void) | void, - deps: Array | void | null, + create: (() => (() => void) | void) | (() => {...} | void | null), + createDeps: Array | void | null, + update?: ((resource: {...} | void | null) => void) | void, + updateDeps?: Array | void | null, + destroy?: ((resource: {...} | void | null) => void) | void, ): void { currentHookNameInDev = 'useEffect'; warnInvalidHookAccess(); updateHookTypesDev(); - return updateEffect(create, deps); + if ( + enableUseEffectCRUDOverload && + (typeof update === 'function' || typeof destroy === 'function') + ) { + return updateResourceEffect( + create, + createDeps, + update, + updateDeps, + destroy, + ); + } else { + return updateEffect(create, createDeps); + } }, useImperativeHandle( ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, @@ -5273,13 +5421,29 @@ if (__DEV__) { return readContext(context); }, useEffect( - create: () => (() => void) | void, - deps: Array | void | null, + create: (() => (() => void) | void) | (() => {...} | void | null), + createDeps: Array | void | null, + update?: ((resource: {...} | void | null) => void) | void, + updateDeps?: Array | void | null, + destroy?: ((resource: {...} | void | null) => void) | void, ): void { currentHookNameInDev = 'useEffect'; warnInvalidHookAccess(); updateHookTypesDev(); - return updateEffect(create, deps); + if ( + enableUseEffectCRUDOverload && + (typeof update === 'function' || typeof destroy === 'function') + ) { + return updateResourceEffect( + create, + createDeps, + update, + updateDeps, + destroy, + ); + } else { + return updateEffect(create, createDeps); + } }, useImperativeHandle( ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index e31b7cefd1..8f35c2fdcb 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -391,8 +391,11 @@ export type Dispatcher = { useContext(context: ReactContext): T, useRef(initialValue: T): {current: T}, useEffect( - create: () => (() => void) | void, - deps: Array | void | null, + create: (() => (() => void) | void) | (() => {...} | void | null), + createDeps: Array | void | null, + update?: ((resource: {...} | void | null) => void) | void, + updateDeps?: Array | void | null, + destroy?: ((resource: {...} | void | null) => void) | void, ): void, // TODO: Non-nullable once `enableUseEffectEventHook` is on everywhere. useEffectEvent?: ) => mixed>(callback: F) => F, diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index e4febdb3e2..4fd0cba0b3 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -41,7 +41,6 @@ let waitFor; let waitForThrow; let waitForPaint; let assertLog; -let useResourceEffect; let assertConsoleErrorDev; describe('ReactHooksWithNoopRenderer', () => { @@ -70,7 +69,6 @@ describe('ReactHooksWithNoopRenderer', () => { useDeferredValue = React.useDeferredValue; Suspense = React.Suspense; Activity = React.unstable_Activity; - useResourceEffect = React.experimental_useResourceEffect; ContinuousEventPriority = require('react-reconciler/constants').ContinuousEventPriority; if (gate(flags => flags.enableSuspenseList)) { @@ -3311,8 +3309,8 @@ describe('ReactHooksWithNoopRenderer', () => { }); }); - // @gate enableUseResourceEffectHook - describe('useResourceEffect', () => { + // @gate enableUseEffectCRUDOverload + describe('useEffect CRUD overload', () => { class Resource { isDeleted: false; id: string; @@ -3333,36 +3331,10 @@ describe('ReactHooksWithNoopRenderer', () => { } } - // @gate !enableUseResourceEffectHook - it('is null when flag is disabled', async () => { - expect(useResourceEffect).toBeUndefined(); - }); - - // @gate enableUseResourceEffectHook - it('validates create return value', async () => { - function App({id}) { - useResourceEffect(() => { - Scheduler.log(`create(${id})`); - }, [id]); - return null; - } - - await act(() => { - ReactNoop.render(); - }); - assertConsoleErrorDev( - [ - 'useResourceEffect must provide a callback which returns a resource. ' + - 'If a managed resource is not needed here, use useEffect. Received undefined', - ], - {withoutStack: true}, - ); - }); - - // @gate enableUseResourceEffectHook + // @gate enableUseEffectCRUDOverload it('validates non-empty update deps', async () => { function App({id}) { - useResourceEffect( + useEffect( () => { Scheduler.log(`create(${id})`); return {}; @@ -3380,19 +3352,19 @@ describe('ReactHooksWithNoopRenderer', () => { ReactNoop.render(); }); assertConsoleErrorDev([ - 'useResourceEffect received a dependency array with no dependencies. ' + + 'useEffect received a dependency array with no dependencies. ' + 'When specified, the dependency array must have at least one dependency.\n' + ' in App (at **)', ]); }); - // @gate enableUseResourceEffectHook + // @gate enableUseEffectCRUDOverload it('simple mount and update', async () => { function App({id, username}) { const opts = useMemo(() => { return {username}; }, [username]); - useResourceEffect( + useEffect( () => { const resource = new Resource(id, opts); Scheduler.log(`create(${resource.id}, ${resource.opts.username})`); @@ -3443,13 +3415,13 @@ describe('ReactHooksWithNoopRenderer', () => { assertLog(['destroy(2, Jack)']); }); - // @gate enableUseResourceEffectHook + // @gate enableUseEffectCRUDOverload it('simple mount with no update', async () => { function App({id, username}) { const opts = useMemo(() => { return {username}; }, [username]); - useResourceEffect( + useEffect( () => { const resource = new Resource(id, opts); Scheduler.log(`create(${resource.id}, ${resource.opts.username})`); @@ -3480,13 +3452,13 @@ describe('ReactHooksWithNoopRenderer', () => { assertLog(['destroy(1, Jack)']); }); - // @gate enableUseResourceEffectHook + // @gate enableUseEffectCRUDOverload it('calls update on every render if no deps are specified', async () => { function App({id, username}) { const opts = useMemo(() => { return {username}; }, [username]); - useResourceEffect( + useEffect( () => { const resource = new Resource(id, opts); Scheduler.log(`create(${resource.id}, ${resource.opts.username})`); @@ -3523,10 +3495,10 @@ describe('ReactHooksWithNoopRenderer', () => { assertLog(['update(2, Lauren)']); }); - // @gate enableUseResourceEffectHook - it('does not unmount previous useResourceEffect between updates', async () => { + // @gate enableUseEffectCRUDOverload + it('does not unmount previous useEffect between updates', async () => { function App({id}) { - useResourceEffect( + useEffect( () => { const resource = new Resource(id); Scheduler.log(`create(${resource.id})`); @@ -3562,10 +3534,10 @@ describe('ReactHooksWithNoopRenderer', () => { assertLog(['update(0)']); }); - // @gate enableUseResourceEffectHook + // @gate enableUseEffectCRUDOverload it('unmounts only on deletion', async () => { function App({id}) { - useResourceEffect( + useEffect( () => { const resource = new Resource(id); Scheduler.log(`create(${resource.id})`); @@ -3596,7 +3568,7 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop).toMatchRenderedOutput(null); }); - // @gate enableUseResourceEffectHook + // @gate enableUseEffectCRUDOverload it('unmounts on deletion', async () => { function Wrapper(props) { return ; @@ -3605,7 +3577,7 @@ describe('ReactHooksWithNoopRenderer', () => { const opts = useMemo(() => { return {username}; }, [username]); - useResourceEffect( + useEffect( () => { const resource = new Resource(id, opts); Scheduler.log(`create(${resource.id}, ${resource.opts.username})`); @@ -3650,10 +3622,10 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop).toMatchRenderedOutput(null); }); - // @gate enableUseResourceEffectHook + // @gate enableUseEffectCRUDOverload it('handles errors in create on mount', async () => { function App({id}) { - useResourceEffect( + useEffect( () => { Scheduler.log(`Mount A [${id}]`); return {}; @@ -3665,7 +3637,7 @@ describe('ReactHooksWithNoopRenderer', () => { Scheduler.log(`Unmount A [${id}]`); }, ); - useResourceEffect( + useEffect( () => { Scheduler.log('Oops!'); throw new Error('Oops!'); @@ -3700,10 +3672,10 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop).toMatchRenderedOutput(null); }); - // @gate enableUseResourceEffectHook + // @gate enableUseEffectCRUDOverload it('handles errors in create on update', async () => { function App({id}) { - useResourceEffect( + useEffect( () => { Scheduler.log(`Mount A [${id}]`); return {}; @@ -3744,13 +3716,13 @@ describe('ReactHooksWithNoopRenderer', () => { }).rejects.toThrow('Oops error!'); }); - // @gate enableUseResourceEffectHook + // @gate enableUseEffectCRUDOverload it('handles errors in destroy on update', async () => { function App({id, username}) { const opts = useMemo(() => { return {username}; }, [username]); - useResourceEffect( + useEffect( () => { const resource = new Resource(id, opts); Scheduler.log(`Mount A [${id}, ${resource.opts.username}]`); @@ -3800,13 +3772,13 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop).toMatchRenderedOutput(null); }); - // @gate enableUseResourceEffectHook && enableActivity + // @gate enableUseEffectCRUDOverload && enableActivity it('composes with activity', async () => { function App({id, username}) { const opts = useMemo(() => { return {username}; }, [username]); - useResourceEffect( + useEffect( () => { const resource = new Resource(id, opts); Scheduler.log(`create(${resource.id}, ${resource.opts.username})`); @@ -3873,7 +3845,7 @@ describe('ReactHooksWithNoopRenderer', () => { assertLog(['destroy(0, Lauren)']); }); - // @gate enableUseResourceEffectHook + // @gate enableUseEffectCRUDOverload it('composes with suspense', async () => { function TextBox({text}) { return ; @@ -3885,7 +3857,7 @@ describe('ReactHooksWithNoopRenderer', () => { const opts = useMemo(() => { return {username}; }, [username]); - useResourceEffect( + useEffect( () => { const resource = new Resource(id, opts); Scheduler.log(`create(${resource.id}, ${resource.opts.username})`); @@ -3991,7 +3963,7 @@ describe('ReactHooksWithNoopRenderer', () => { ); }); - // @gate enableUseResourceEffectHook + // @gate enableUseEffectCRUDOverload it('composes with other kinds of effects', async () => { let rerender; function App({id, username}) { @@ -4003,7 +3975,7 @@ describe('ReactHooksWithNoopRenderer', () => { useEffect(() => { Scheduler.log(`useEffect(${count})`); }, [count]); - useResourceEffect( + useEffect( () => { const resource = new Resource(id, opts); Scheduler.log(`create(${resource.id}, ${resource.opts.username})`); diff --git a/packages/react/src/ReactHooks.js b/packages/react/src/ReactHooks.js index 677e0d705b..2fe6d2b85b 100644 --- a/packages/react/src/ReactHooks.js +++ b/packages/react/src/ReactHooks.js @@ -87,11 +87,31 @@ export function useRef(initialValue: T): {current: T} { } export function useEffect( - create: () => (() => void) | void, - deps: Array | void | null, + create: (() => (() => void) | void) | (() => {...} | void | null), + createDeps: Array | void | null, + update?: ((resource: {...} | void | null) => void) | void, + updateDeps?: Array | void | null, + destroy?: ((resource: {...} | void | null) => void) | void, ): void { const dispatcher = resolveDispatcher(); - return dispatcher.useEffect(create, deps); + if ( + enableUseEffectCRUDOverload && + (typeof update === 'function' || typeof destroy === 'function') + ) { + // $FlowFixMe[not-a-function] This is unstable, thus optional + return dispatcher.useEffect( + create, + createDeps, + update, + updateDeps, + destroy, + ); + } else if (typeof update === 'function') { + throw new Error( + 'useEffect CRUD overload is not enabled in this build of React.', + ); + } + return dispatcher.useEffect(create, createDeps); } export function useInsertionEffect( diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js b/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js index b6e3b098db..8437622a9b 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js @@ -25,6 +25,6 @@ export const enableShallowPropDiffing = __VARIANT__; export const passChildrenWhenCloningPersistedNodes = __VARIANT__; export const enableFabricCompleteRootInCommitPhase = __VARIANT__; export const enableSiblingPrerendering = __VARIANT__; -export const enableUseResourceEffectHook = __VARIANT__; +export const enableUseEffectCRUDOverload = __VARIANT__; export const enableOwnerStacks = __VARIANT__; export const enableRemoveConsolePatches = __VARIANT__; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index c1d23e898c..9d48a8e2b1 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -25,7 +25,7 @@ export const { enableObjectFiber, enablePersistedModeClonedFlag, enableShallowPropDiffing, - enableUseResourceEffectHook, + enableUseEffectCRUDOverload, passChildrenWhenCloningPersistedNodes, enableSiblingPrerendering, enableOwnerStacks, diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index afe18430eb..e052c82752 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -64,7 +64,7 @@ export const retryLaneExpirationMs = 5000; export const syncLaneExpirationMs = 250; export const transitionLaneExpirationMs = 5000; export const enableSiblingPrerendering = true; -export const enableUseResourceEffectHook = false; +export const enableUseEffectCRUDOverload = false; export const enableHydrationLaneScheduling = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 5b4c33f331..8e47372fb7 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -65,7 +65,7 @@ export const renameElementSymbol = true; export const enableShallowPropDiffing = false; export const enableSiblingPrerendering = true; -export const enableUseResourceEffectHook = false; +export const enableUseEffectCRUDOverload = false; export const enableYieldingBeforePassive = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js index ebc8c5eb97..02639e1be0 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js @@ -63,7 +63,7 @@ export const syncLaneExpirationMs = 250; export const transitionLaneExpirationMs = 5000; export const enableFabricCompleteRootInCommitPhase = false; export const enableSiblingPrerendering = true; -export const enableUseResourceEffectHook = true; +export const enableUseEffectCRUDOverload = true; export const enableHydrationLaneScheduling = true; export const enableYieldingBeforePassive = false; export const enableThrottledScheduling = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index c7c0e62730..1b1fc04a3f 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -75,7 +75,7 @@ export const enableOwnerStacks = false; export const enableShallowPropDiffing = false; export const enableSiblingPrerendering = true; -export const enableUseResourceEffectHook = false; +export const enableUseEffectCRUDOverload = false; export const enableHydrationLaneScheduling = true; diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index b75a0a9175..b9ad777265 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -36,7 +36,7 @@ export const enableSchedulingProfiler = __VARIANT__; export const enableInfiniteRenderLoopDetection = __VARIANT__; export const enableSiblingPrerendering = __VARIANT__; -export const enableUseResourceEffectHook = __VARIANT__; +export const enableUseEffectCRUDOverload = __VARIANT__; export const enableRemoveConsolePatches = __VARIANT__; // TODO: These flags are hard-coded to the default values used in open source. diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 58a7bcd26f..ed8b9f8c9c 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -29,7 +29,7 @@ export const { enableSiblingPrerendering, enableTransitionTracing, enableTrustedTypesIntegration, - enableUseResourceEffectHook, + enableUseEffectCRUDOverload, favorSafetyOverHydrationPerf, renameElementSymbol, retryLaneExpirationMs, diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json index 8fa3d190ec..6ab654f1d3 100644 --- a/scripts/error-codes/codes.json +++ b/scripts/error-codes/codes.json @@ -530,5 +530,6 @@ "542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary.", "543": "Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.", "544": "Found a pair with an auto name. This is a bug in React.", - "545": "The %s tag may only be rendered once." + "545": "The %s tag may only be rendered once.", + "546": "useEffect CRUD overload is not enabled in this build of React." } From ea28f29199083452e997929fd1a54daed42e2d36 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Thu, 23 Jan 2025 16:55:13 -0500 Subject: [PATCH 19/19] [crud] Remove useResourceEffect Removes useResourceEffect. --- .../react-debug-tools/src/ReactDebugHooks.js | 22 --- .../react-reconciler/src/ReactFiberHooks.js | 158 ------------------ .../src/ReactInternalTypes.js | 9 - packages/react-server/src/ReactFizzHooks.js | 10 +- packages/react/index.development.js | 1 - .../react/index.experimental.development.js | 1 - packages/react/index.fb.js | 1 - packages/react/src/ReactClient.js | 5 - packages/react/src/ReactHooks.js | 21 --- 9 files changed, 1 insertion(+), 227 deletions(-) diff --git a/packages/react-debug-tools/src/ReactDebugHooks.js b/packages/react-debug-tools/src/ReactDebugHooks.js index f45ccfecdc..114080d03e 100644 --- a/packages/react-debug-tools/src/ReactDebugHooks.js +++ b/packages/react-debug-tools/src/ReactDebugHooks.js @@ -128,9 +128,6 @@ function getPrimitiveStackCache(): Map> { Dispatcher.useId(); - if (typeof Dispatcher.useResourceEffect === 'function') { - Dispatcher.useResourceEffect(() => ({}), []); - } if (typeof Dispatcher.useEffectEvent === 'function') { Dispatcher.useEffectEvent((args: empty) => {}); } @@ -741,24 +738,6 @@ function useHostTransitionStatus(): TransitionStatus { return status; } -function useResourceEffect( - create: () => {...} | void | null, - createDeps: Array | void | null, - update: ((resource: {...} | void | null) => void) | void, - updateDeps: Array | void | null, - destroy: ((resource: {...} | void | null) => void) | void, -) { - nextHook(); - hookLog.push({ - displayName: null, - primitive: 'ResourceEffect', - stackError: new Error(), - value: create, - debugInfo: null, - dispatcherHookName: 'ResourceEffect', - }); -} - function useEffectEvent) => mixed>(callback: F): F { nextHook(); hookLog.push({ @@ -798,7 +777,6 @@ const Dispatcher: DispatcherType = { useActionState, useHostTransitionStatus, useEffectEvent, - useResourceEffect, }; // create a proxy to throw a custom error diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 110f896deb..71a6d51843 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -3989,9 +3989,6 @@ export const ContextOnlyDispatcher: Dispatcher = { if (enableUseEffectEventHook) { (ContextOnlyDispatcher: Dispatcher).useEffectEvent = throwInvalidHookError; } -if (enableUseEffectCRUDOverload) { - (ContextOnlyDispatcher: Dispatcher).useResourceEffect = throwInvalidHookError; -} const HooksDispatcherOnMount: Dispatcher = { readContext, @@ -4022,9 +4019,6 @@ const HooksDispatcherOnMount: Dispatcher = { if (enableUseEffectEventHook) { (HooksDispatcherOnMount: Dispatcher).useEffectEvent = mountEvent; } -if (enableUseEffectCRUDOverload) { - (HooksDispatcherOnMount: Dispatcher).useResourceEffect = mountResourceEffect; -} const HooksDispatcherOnUpdate: Dispatcher = { readContext, @@ -4055,10 +4049,6 @@ const HooksDispatcherOnUpdate: Dispatcher = { if (enableUseEffectEventHook) { (HooksDispatcherOnUpdate: Dispatcher).useEffectEvent = updateEvent; } -if (enableUseEffectCRUDOverload) { - (HooksDispatcherOnUpdate: Dispatcher).useResourceEffect = - updateResourceEffect; -} const HooksDispatcherOnRerender: Dispatcher = { readContext, @@ -4089,10 +4079,6 @@ const HooksDispatcherOnRerender: Dispatcher = { if (enableUseEffectEventHook) { (HooksDispatcherOnRerender: Dispatcher).useEffectEvent = updateEvent; } -if (enableUseEffectCRUDOverload) { - (HooksDispatcherOnRerender: Dispatcher).useResourceEffect = - updateResourceEffect; -} let HooksDispatcherOnMountInDEV: Dispatcher | null = null; let HooksDispatcherOnMountWithHookTypesInDEV: Dispatcher | null = null; @@ -4310,27 +4296,6 @@ if (__DEV__) { return mountEvent(callback); }; } - if (enableUseEffectCRUDOverload) { - (HooksDispatcherOnMountInDEV: Dispatcher).useResourceEffect = - function useResourceEffect( - create: () => {...} | void | null, - createDeps: Array | void | null, - update: ((resource: {...} | void | null) => void) | void, - updateDeps: Array | void | null, - destroy: ((resource: {...} | void | null) => void) | void, - ): void { - currentHookNameInDev = 'useResourceEffect'; - mountHookTypesDev(); - checkDepsAreNonEmptyArrayDev(updateDeps); - return mountResourceEffect( - create, - createDeps, - update, - updateDeps, - destroy, - ); - }; - } HooksDispatcherOnMountWithHookTypesInDEV = { readContext(context: ReactContext): T { @@ -4514,26 +4479,6 @@ if (__DEV__) { return mountEvent(callback); }; } - if (enableUseEffectCRUDOverload) { - (HooksDispatcherOnMountWithHookTypesInDEV: Dispatcher).useResourceEffect = - function useResourceEffect( - create: () => {...} | void | null, - createDeps: Array | void | null, - update: ((resource: {...} | void | null) => void) | void, - updateDeps: Array | void | null, - destroy: ((resource: {...} | void | null) => void) | void, - ): void { - currentHookNameInDev = 'useResourceEffect'; - updateHookTypesDev(); - return mountResourceEffect( - create, - createDeps, - update, - updateDeps, - destroy, - ); - }; - } HooksDispatcherOnUpdateInDEV = { readContext(context: ReactContext): T { @@ -4717,26 +4662,6 @@ if (__DEV__) { return updateEvent(callback); }; } - if (enableUseEffectCRUDOverload) { - (HooksDispatcherOnUpdateInDEV: Dispatcher).useResourceEffect = - function useResourceEffect( - create: () => {...} | void | null, - createDeps: Array | void | null, - update: ((resource: {...} | void | null) => void) | void, - updateDeps: Array | void | null, - destroy: ((resource: {...} | void | null) => void) | void, - ) { - currentHookNameInDev = 'useResourceEffect'; - updateHookTypesDev(); - return updateResourceEffect( - create, - createDeps, - update, - updateDeps, - destroy, - ); - }; - } HooksDispatcherOnRerenderInDEV = { readContext(context: ReactContext): T { @@ -4920,26 +4845,6 @@ if (__DEV__) { return updateEvent(callback); }; } - if (enableUseEffectCRUDOverload) { - (HooksDispatcherOnRerenderInDEV: Dispatcher).useResourceEffect = - function useResourceEffect( - create: () => {...} | void | null, - createDeps: Array | void | null, - update: ((resource: {...} | void | null) => void) | void, - updateDeps: Array | void | null, - destroy: ((resource: {...} | void | null) => void) | void, - ) { - currentHookNameInDev = 'useResourceEffect'; - updateHookTypesDev(); - return updateResourceEffect( - create, - createDeps, - update, - updateDeps, - destroy, - ); - }; - } InvalidNestedHooksDispatcherOnMountInDEV = { readContext(context: ReactContext): T { @@ -5148,27 +5053,6 @@ if (__DEV__) { return mountEvent(callback); }; } - if (enableUseEffectCRUDOverload) { - (InvalidNestedHooksDispatcherOnMountInDEV: Dispatcher).useResourceEffect = - function useResourceEffect( - create: () => {...} | void | null, - createDeps: Array | void | null, - update: ((resource: {...} | void | null) => void) | void, - updateDeps: Array | void | null, - destroy: ((resource: {...} | void | null) => void) | void, - ): void { - currentHookNameInDev = 'useResourceEffect'; - warnInvalidHookAccess(); - mountHookTypesDev(); - return mountResourceEffect( - create, - createDeps, - update, - updateDeps, - destroy, - ); - }; - } InvalidNestedHooksDispatcherOnUpdateInDEV = { readContext(context: ReactContext): T { @@ -5377,27 +5261,6 @@ if (__DEV__) { return updateEvent(callback); }; } - if (enableUseEffectCRUDOverload) { - (InvalidNestedHooksDispatcherOnUpdateInDEV: Dispatcher).useResourceEffect = - function useResourceEffect( - create: () => {...} | void | null, - createDeps: Array | void | null, - update: ((resource: {...} | void | null) => void) | void, - updateDeps: Array | void | null, - destroy: ((resource: {...} | void | null) => void) | void, - ) { - currentHookNameInDev = 'useResourceEffect'; - warnInvalidHookAccess(); - updateHookTypesDev(); - return updateResourceEffect( - create, - createDeps, - update, - updateDeps, - destroy, - ); - }; - } InvalidNestedHooksDispatcherOnRerenderInDEV = { readContext(context: ReactContext): T { @@ -5606,25 +5469,4 @@ if (__DEV__) { return updateEvent(callback); }; } - if (enableUseEffectCRUDOverload) { - (InvalidNestedHooksDispatcherOnRerenderInDEV: Dispatcher).useResourceEffect = - function useResourceEffect( - create: () => {...} | void | null, - createDeps: Array | void | null, - update: ((resource: {...} | void | null) => void) | void, - updateDeps: Array | void | null, - destroy: ((resource: {...} | void | null) => void) | void, - ) { - currentHookNameInDev = 'useResourceEffect'; - warnInvalidHookAccess(); - updateHookTypesDev(); - return updateResourceEffect( - create, - createDeps, - update, - updateDeps, - destroy, - ); - }; - } } diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index 8f35c2fdcb..98e7d4deef 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -47,7 +47,6 @@ export type HookType = | 'useRef' | 'useEffect' | 'useEffectEvent' - | 'useResourceEffect' | 'useInsertionEffect' | 'useLayoutEffect' | 'useCallback' @@ -399,14 +398,6 @@ export type Dispatcher = { ): void, // TODO: Non-nullable once `enableUseEffectEventHook` is on everywhere. useEffectEvent?: ) => mixed>(callback: F) => F, - // TODO: Non-nullable once `enableUseResourceEffectHook` is on everywhere. - useResourceEffect?: ( - create: () => {...} | void | null, - createDeps: Array | void | null, - update: ((resource: {...} | void | null) => void) | void, - updateDeps: Array | void | null, - destroy: ((resource: {...} | void | null) => void) | void, - ) => void, useInsertionEffect( create: () => (() => void) | void, deps: Array | void | null, diff --git a/packages/react-server/src/ReactFizzHooks.js b/packages/react-server/src/ReactFizzHooks.js index 63e8576ca7..a0ec1c7414 100644 --- a/packages/react-server/src/ReactFizzHooks.js +++ b/packages/react-server/src/ReactFizzHooks.js @@ -38,10 +38,7 @@ import { } from './ReactFizzConfig'; import {createFastHash} from './ReactServerStreamConfig'; -import { - enableUseEffectEventHook, - enableUseEffectCRUDOverload, -} from 'shared/ReactFeatureFlags'; +import {enableUseEffectEventHook} from 'shared/ReactFeatureFlags'; import is from 'shared/objectIs'; import { REACT_CONTEXT_TYPE, @@ -866,11 +863,6 @@ export const HooksDispatcher: Dispatcher = supportsClientAPIs if (enableUseEffectEventHook) { HooksDispatcher.useEffectEvent = useEffectEvent; } -if (enableUseEffectCRUDOverload) { - HooksDispatcher.useResourceEffect = supportsClientAPIs - ? noop - : clientHookNotSupported; -} export let currentResumableState: null | ResumableState = (null: any); export function setCurrentResumableState( diff --git a/packages/react/index.development.js b/packages/react/index.development.js index 809e940f07..fa79633001 100644 --- a/packages/react/index.development.js +++ b/packages/react/index.development.js @@ -59,7 +59,6 @@ export { useDeferredValue, useEffect, experimental_useEffectEvent, - experimental_useResourceEffect, useImperativeHandle, useInsertionEffect, useLayoutEffect, diff --git a/packages/react/index.experimental.development.js b/packages/react/index.experimental.development.js index 49c98bb208..6074b683b7 100644 --- a/packages/react/index.experimental.development.js +++ b/packages/react/index.experimental.development.js @@ -42,7 +42,6 @@ export { useDeferredValue, useEffect, experimental_useEffectEvent, - experimental_useResourceEffect, useImperativeHandle, useInsertionEffect, useLayoutEffect, diff --git a/packages/react/index.fb.js b/packages/react/index.fb.js index 84128cf0ea..8b97f85d9d 100644 --- a/packages/react/index.fb.js +++ b/packages/react/index.fb.js @@ -21,7 +21,6 @@ export { createElement, createRef, experimental_useEffectEvent, - experimental_useResourceEffect, forwardRef, Fragment, isValidElement, diff --git a/packages/react/src/ReactClient.js b/packages/react/src/ReactClient.js index 2bacbc8567..715ea8ab47 100644 --- a/packages/react/src/ReactClient.js +++ b/packages/react/src/ReactClient.js @@ -41,7 +41,6 @@ import { useContext, useEffect, useEffectEvent, - useResourceEffect, useImperativeHandle, useDebugValue, useInsertionEffect, @@ -65,7 +64,6 @@ import {addTransitionType} from './ReactTransitionType'; import {act} from './ReactAct'; import {captureOwnerStack} from './ReactOwnerStack'; import * as ReactCompilerRuntime from './ReactCompilerRuntime'; -import {enableUseEffectCRUDOverload} from 'shared/ReactFeatureFlags'; const Children = { map, @@ -132,6 +130,3 @@ export { act, // DEV-only captureOwnerStack, // DEV-only }; - -export const experimental_useResourceEffect: typeof useResourceEffect | void = - enableUseEffectCRUDOverload ? useResourceEffect : undefined; diff --git a/packages/react/src/ReactHooks.js b/packages/react/src/ReactHooks.js index 2fe6d2b85b..06d61d2238 100644 --- a/packages/react/src/ReactHooks.js +++ b/packages/react/src/ReactHooks.js @@ -221,27 +221,6 @@ export function useEffectEvent) => mixed>( return dispatcher.useEffectEvent(callback); } -export function useResourceEffect( - create: () => {...} | void | null, - createDeps: Array | void | null, - update: ((resource: {...} | void | null) => void) | void, - updateDeps: Array | void | null, - destroy: ((resource: {...} | void | null) => void) | void, -): void { - if (!enableUseEffectCRUDOverload) { - throw new Error('Not implemented.'); - } - const dispatcher = resolveDispatcher(); - // $FlowFixMe[not-a-function] This is unstable, thus optional - return dispatcher.useResourceEffect( - create, - createDeps, - update, - updateDeps, - destroy, - ); -} - export function useOptimistic( passthrough: S, reducer: ?(S, A) => S,