From e3ebcd54b98a4f8f5a9f7e63982fa75578b648ed Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 5 Apr 2024 10:53:11 -0400 Subject: [PATCH 1/4] Move string ref coercion to JSX runtime (#28473) Based on: - #28464 --- This moves the entire string ref implementation out Fiber and into the JSX runtime. The string is converted to a callback ref during element creation. This is a subtle change in behavior, because it will have already been converted to a callback ref if you access element.prop.ref or element.ref. But this is only for Meta, because string refs are disabled entirely in open source. And if it leads to an issue in practice, the solution is to switch to a different ref type, which Meta is going to do regardless. --- .../src/__tests__/ReactComponent-test.js | 15 +- .../ReactDOMServerIntegrationRefs-test.js | 1 + .../ReactDeprecationWarnings-test.js | 9 +- .../__tests__/ReactFunctionComponent-test.js | 16 +- .../multiple-copies-of-react-test.js | 12 +- packages/react-dom/src/__tests__/refs-test.js | 73 ++++---- .../react-reconciler/src/ReactChildFiber.js | 156 +----------------- .../ReactIncrementalSideEffects-test.js | 1 + .../ReactCoffeeScriptClass-test.coffee | 2 +- .../react/src/__tests__/ReactES6Class-test.js | 2 +- .../src/__tests__/ReactElementClone-test.js | 10 +- .../src/__tests__/ReactStrictMode-test.js | 8 +- .../__tests__/ReactTypeScriptClass-test.ts | 2 +- packages/react/src/jsx/ReactJSXElement.js | 137 ++++++++++++++- 14 files changed, 199 insertions(+), 245 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactComponent-test.js b/packages/react-dom/src/__tests__/ReactComponent-test.js index 1a77cbad680a..42dae06370ed 100644 --- a/packages/react-dom/src/__tests__/ReactComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactComponent-test.js @@ -47,10 +47,9 @@ describe('ReactComponent', () => { act(() => { root.render(
); }), - ).rejects.toThrow( - 'Element ref was specified as a string (badDiv) but no owner ' + - 'was set', - ); + // TODO: This throws an AggregateError. Need to update test infra to + // support matching against AggregateError. + ).rejects.toThrow(); }); it('should throw (in dev) when children are mutated during render', async () => { @@ -169,18 +168,14 @@ describe('ReactComponent', () => { root.render(); }); }).toErrorDev([ - 'Warning: Component "div" contains the string ref "inner". ' + + 'Warning: Component "Component" contains the string ref "inner". ' + 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' + + ' in Wrapper (at **)\n' + ' in div (at **)\n' + ' in Wrapper (at **)\n' + ' in Component (at **)', - 'Warning: Component "Component" contains the string ref "outer". ' + - 'Support for string refs will be removed in a future major release. ' + - 'We recommend using useRef() or createRef() instead. ' + - 'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' + - ' in Component (at **)', ]); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js index b7179d55bda2..4278e3298441 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js @@ -100,6 +100,7 @@ describe('ReactDOMServerIntegration', () => { 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' + + ' in div (at **)\n' + ' in RefsComponent (at **)', ]); expect(component.refs.myDiv).toBe(root.firstChild); diff --git a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js index 39a068e1c7ed..009ae14fccaa 100644 --- a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js +++ b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js @@ -86,6 +86,7 @@ describe('ReactDeprecationWarnings', () => { 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: ' + 'https://react.dev/link/strict-mode-string-ref' + + '\n in RefComponent (at **)' + '\n in Component (at **)', ); }); @@ -137,10 +138,6 @@ describe('ReactDeprecationWarnings', () => { 'We ask you to manually fix this case by using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: ' + 'https://react.dev/link/strict-mode-string-ref', - 'Warning: Component "Component" contains the string ref "refComponent". ' + - 'Support for string refs will be removed in a future major release. We recommend ' + - 'using useRef() or createRef() instead. Learn more about using refs safely here: ' + - 'https://react.dev/link/strict-mode-string-ref', ]); }); @@ -173,10 +170,6 @@ describe('ReactDeprecationWarnings', () => { 'We ask you to manually fix this case by using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: ' + 'https://react.dev/link/strict-mode-string-ref', - 'Warning: Component "Component" contains the string ref "refComponent". ' + - 'Support for string refs will be removed in a future major release. We recommend ' + - 'using useRef() or createRef() instead. Learn more about using refs safely here: ' + - 'https://react.dev/link/strict-mode-string-ref', ]); }); }); diff --git a/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js b/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js index d353f6f8c6ee..813cc9fb6e67 100644 --- a/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js @@ -185,18 +185,10 @@ describe('ReactFunctionComponent', () => { act(() => { root.render(); }), - ).rejects.toThrowError( - __DEV__ - ? 'Function components cannot have string refs. We recommend using useRef() instead.' - : // It happens because we don't save _owner in production for - // function components. - 'Element ref was specified as a string (me) but no owner was set. This could happen for one of' + - ' the following reasons:\n' + - '1. You may be adding a ref to a function component\n' + - "2. You may be adding a ref to a component that was not created inside a component's render method\n" + - '3. You have multiple copies of React loaded\n' + - 'See https://react.dev/link/refs-must-have-owner for more information.', - ); + ) + // TODO: This throws an AggregateError. Need to update test infra to + // support matching against AggregateError. + .rejects.toThrowError(); }); // @gate !enableRefAsProp || !__DEV__ diff --git a/packages/react-dom/src/__tests__/multiple-copies-of-react-test.js b/packages/react-dom/src/__tests__/multiple-copies-of-react-test.js index 0d63137b1ab7..2d107cbb1970 100644 --- a/packages/react-dom/src/__tests__/multiple-copies-of-react-test.js +++ b/packages/react-dom/src/__tests__/multiple-copies-of-react-test.js @@ -30,13 +30,9 @@ describe('when different React version is used with string ref', () => { act(() => { root.render(); }), - ).rejects.toThrow( - 'Element ref was specified as a string (foo) but no owner was set. This could happen for one of' + - ' the following reasons:\n' + - '1. You may be adding a ref to a function component\n' + - "2. You may be adding a ref to a component that was not created inside a component's render method\n" + - '3. You have multiple copies of React loaded\n' + - 'See https://react.dev/link/refs-must-have-owner for more information.', - ); + ) + // TODO: This throws an AggregateError. Need to update test infra to + // support matching against AggregateError. + .rejects.toThrow(); }); }); diff --git a/packages/react-dom/src/__tests__/refs-test.js b/packages/react-dom/src/__tests__/refs-test.js index 4a638ef17c56..a24bdf481e16 100644 --- a/packages/react-dom/src/__tests__/refs-test.js +++ b/packages/react-dom/src/__tests__/refs-test.js @@ -129,22 +129,22 @@ describe('reactiverefs', () => { ); }); }).toErrorDev([ - 'Warning: Component "div" contains the string ref "resetDiv". ' + - 'Support for string refs will be removed in a future major release. ' + - 'We recommend using useRef() or createRef() instead. ' + - 'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' + - ' in div (at **)\n' + - ' in TestRefsComponent (at **)', - 'Warning: Component "span" contains the string ref "clickLog0". ' + - 'Support for string refs will be removed in a future major release. ' + - 'We recommend using useRef() or createRef() instead. ' + - 'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' + - ' in span (at **)\n' + - ' in ClickCounter (at **)\n' + + 'Warning: Component "TestRefsComponent" contains the string ' + + 'ref "resetDiv". Support for string refs will be removed in a ' + + 'future major release. We recommend using useRef() or createRef() ' + + 'instead. Learn more about using refs safely ' + + 'here: https://react.dev/link/strict-mode-string-ref\n' + ' in div (at **)\n' + - ' in GeneralContainerComponent (at **)\n' + ' in div (at **)\n' + ' in TestRefsComponent (at **)', + 'Warning: Component "ClickCounter" contains the string ' + + 'ref "clickLog0". Support for string refs will be removed in a ' + + 'future major release. We recommend using useRef() or createRef() ' + + 'instead. Learn more about using refs safely ' + + 'here: https://react.dev/link/strict-mode-string-ref\n' + + ' in div (at **)\n' + + ' in span (at **)\n' + + ' in ClickCounter (at **)', ]); expect(testRefsComponent instanceof TestRefsComponent).toBe(true); @@ -352,12 +352,12 @@ describe('ref swapping', () => { 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' + + ' in div (at **)\n' + ' in A (at **)', ]); expect(a.refs[1].nodeName).toBe('DIV'); }); - // @gate !disableStringRefs it('provides an error for invalid refs', async () => { const container = document.createElement('div'); const root = ReactDOMClient.createRoot(container); @@ -365,16 +365,16 @@ describe('ref swapping', () => { await act(() => { root.render(
); }); - }).rejects.toThrow( - 'Element ref was specified as a string (10) but no owner was set.', - ); + // TODO: This throws an AggregateError. Need to update test infra to + // support matching against AggregateError. + }).rejects.toThrow(); await expect(async () => { await act(() => { root.render(
); }); - }).rejects.toThrow( - 'Element ref was specified as a string (true) but no owner was set.', - ); + // TODO: This throws an AggregateError. Need to update test infra to + // support matching against AggregateError. + }).rejects.toThrow(); await expect(async () => { await act(() => { root.render(
); @@ -520,14 +520,10 @@ describe('creating element with string ref in constructor', () => { await act(() => { root.render(); }); - }).rejects.toThrowError( - 'Element ref was specified as a string (p) but no owner was set. This could happen for one of' + - ' the following reasons:\n' + - '1. You may be adding a ref to a function component\n' + - "2. You may be adding a ref to a component that was not created inside a component's render method\n" + - '3. You have multiple copies of React loaded\n' + - 'See https://react.dev/link/refs-must-have-owner for more information.', - ); + }) + // TODO: This throws an AggregateError. Need to update test infra to + // support matching against AggregateError. + .rejects.toThrowError(); }); }); @@ -581,10 +577,11 @@ describe('strings refs across renderers', () => { ); }); }).toErrorDev([ - 'Warning: Component "Indirection" contains the string ref "child1". ' + + 'Warning: Component "Parent" contains the string ref "child1". ' + 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' + + ' in div (at **)\n' + ' in Indirection (at **)\n' + ' in Parent (at **)', ]); @@ -593,20 +590,10 @@ describe('strings refs across renderers', () => { expect(inst.refs.child1.tagName).toBe('DIV'); expect(inst.refs.child1).toBe(div1.firstChild); - await expect(async () => { - // Now both refs should be rendered. - await act(() => { - root.render(); - }); - }).toErrorDev( - [ - 'Warning: Component "Root" contains the string ref "child2". ' + - 'Support for string refs will be removed in a future major release. ' + - 'We recommend using useRef() or createRef() instead. ' + - 'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref', - ], - {withoutStack: true}, - ); + // Now both refs should be rendered. + await act(() => { + root.render(); + }); expect(inst.refs.child1.tagName).toBe('DIV'); expect(inst.refs.child1).toBe(div1.firstChild); expect(inst.refs.child2.tagName).toBe('DIV'); diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index dde892c5893e..742206c47fbf 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -33,17 +33,9 @@ import { REACT_LAZY_TYPE, REACT_CONTEXT_TYPE, } from 'shared/ReactSymbols'; -import { - ClassComponent, - HostRoot, - HostText, - HostPortal, - Fragment, -} from './ReactWorkTags'; +import {HostRoot, HostText, HostPortal, Fragment} from './ReactWorkTags'; import isArray from 'shared/isArray'; -import assign from 'shared/assign'; -import {checkPropStringCoercion} from 'shared/CheckStringCoercion'; -import {enableRefAsProp, disableStringRefs} from 'shared/ReactFeatureFlags'; +import {enableRefAsProp} from 'shared/ReactFeatureFlags'; import { createWorkInProgress, @@ -84,7 +76,6 @@ function mergeDebugInfo( let didWarnAboutMaps; let didWarnAboutGenerators; -let didWarnAboutStringRefs; let ownerHasKeyUseWarning; let ownerHasFunctionTypeWarning; let ownerHasSymbolTypeWarning; @@ -93,7 +84,6 @@ let warnForMissingKey = (child: mixed, returnFiber: Fiber) => {}; if (__DEV__) { didWarnAboutMaps = false; didWarnAboutGenerators = false; - didWarnAboutStringRefs = ({}: {[string]: boolean}); /** * Warn if there's no key explicitly set on dynamic arrays of children or @@ -137,10 +127,6 @@ if (__DEV__) { }; } -function isReactClass(type: any) { - return type.prototype && type.prototype.isReactComponent; -} - function unwrapThenable(thenable: Thenable): T { const index = thenableIndexCounter; thenableIndexCounter += 1; @@ -150,157 +136,27 @@ function unwrapThenable(thenable: Thenable): T { return trackUsedThenable(thenableState, thenable, index); } -type CoercedStringRef = ((handle: mixed) => void) & {_stringRef: ?string, ...}; - -function convertStringRefToCallbackRef( - returnFiber: Fiber, - current: Fiber | null, - element: ReactElement, - mixedRef: string | number | boolean, -): CoercedStringRef { - if (__DEV__) { - checkPropStringCoercion(mixedRef, 'ref'); - } - const stringRef = '' + (mixedRef: any); - - const owner: ?Fiber = (element._owner: any); - if (!owner) { - throw new Error( - `Element ref was specified as a string (${stringRef}) but no owner was set. This could happen for one of` + - ' the following reasons:\n' + - '1. You may be adding a ref to a function component\n' + - "2. You may be adding a ref to a component that was not created inside a component's render method\n" + - '3. You have multiple copies of React loaded\n' + - 'See https://react.dev/link/refs-must-have-owner for more information.', - ); - } - if (owner.tag !== ClassComponent) { - throw new Error( - 'Function components cannot have string refs. ' + - 'We recommend using useRef() instead. ' + - 'Learn more about using refs safely here: ' + - 'https://react.dev/link/strict-mode-string-ref', - ); - } - - if (__DEV__) { - if ( - // Will already warn with "Function components cannot be given refs" - !(typeof element.type === 'function' && !isReactClass(element.type)) - ) { - const componentName = - getComponentNameFromFiber(returnFiber) || 'Component'; - if (!didWarnAboutStringRefs[componentName]) { - console.error( - 'Component "%s" contains the string ref "%s". Support for string refs ' + - 'will be removed in a future major release. We recommend using ' + - 'useRef() or createRef() instead. ' + - 'Learn more about using refs safely here: ' + - 'https://react.dev/link/strict-mode-string-ref', - componentName, - stringRef, - ); - didWarnAboutStringRefs[componentName] = true; - } - } - } - - const inst = owner.stateNode; - if (!inst) { - throw new Error( - `Missing owner for string ref ${stringRef}. This error is likely caused by a ` + - 'bug in React. Please file an issue.', - ); - } - - // Check if previous string ref matches new string ref - if ( - current !== null && - current.ref !== null && - typeof current.ref === 'function' && - current.ref._stringRef === stringRef - ) { - // Reuse the existing string ref - const currentRef: CoercedStringRef = ((current.ref: any): CoercedStringRef); - return currentRef; - } - - // Create a new string ref - const ref = function (value: mixed) { - const refs = inst.refs; - if (value === null) { - delete refs[stringRef]; - } else { - refs[stringRef] = value; - } - }; - ref._stringRef = stringRef; - return ref; -} - function coerceRef( returnFiber: Fiber, current: Fiber | null, workInProgress: Fiber, element: ReactElement, ): void { - let mixedRef; + let ref; if (enableRefAsProp) { // TODO: This is a temporary, intermediate step. When enableRefAsProp is on, // we should resolve the `ref` prop during the begin phase of the component // it's attached to (HostComponent, ClassComponent, etc). const refProp = element.props.ref; - mixedRef = refProp !== undefined ? refProp : null; + ref = refProp !== undefined ? refProp : null; } else { // Old behavior. - mixedRef = element.ref; - } - - let coercedRef; - if ( - !disableStringRefs && - (typeof mixedRef === 'string' || - typeof mixedRef === 'number' || - typeof mixedRef === 'boolean') - ) { - coercedRef = convertStringRefToCallbackRef( - returnFiber, - current, - element, - mixedRef, - ); - - if (enableRefAsProp) { - // When enableRefAsProp is on, we should always use the props as the - // source of truth for refs. Not a field on the fiber. - // - // In the case of string refs, this presents a problem, because string - // refs are not passed around internally as strings; they are converted to - // callback refs. The ref used by the reconciler is not the same as the - // one the user provided. - // - // But since this is a deprecated feature anyway, what we can do is clone - // the props object and replace it with the internal callback ref. Then we - // can continue to use the props object as the source of truth. - // - // This means the internal callback ref will leak into userspace. The - // receiving component will receive a callback ref even though the parent - // passed a string. Which is weird, but again, this is a deprecated - // feature, and we're only leaving it around behind a flag so that Meta - // can keep using string refs temporarily while they finish migrating - // their codebase. - const userProvidedProps = workInProgress.pendingProps; - const propsWithInternalCallbackRef = assign({}, userProvidedProps); - propsWithInternalCallbackRef.ref = coercedRef; - workInProgress.pendingProps = propsWithInternalCallbackRef; - } - } else { - coercedRef = mixedRef; + ref = element.ref; } // TODO: If enableRefAsProp is on, we shouldn't use the `ref` field. We // should always read the ref from the prop. - workInProgress.ref = coercedRef; + workInProgress.ref = ref; } function throwOnInvalidObjectType(returnFiber: Fiber, newChild: Object) { diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js index 9f892d3f91f3..6226e2bc228f 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js @@ -1374,6 +1374,7 @@ describe('ReactIncrementalSideEffects', () => { 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' + + ' in Bar (at **)\n' + ' in Foo (at **)', ]); expect(fooInstance.refs.bar.test).toEqual('test'); diff --git a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee index f29f38a128b3..033156d25f0f 100644 --- a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee +++ b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee @@ -558,7 +558,7 @@ describe 'ReactCoffeeScriptClass', -> 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' + - ' in Foo (at **)' + ' in _Class (at **)' ]); expect(ref.current.refs.inner.getName()).toBe 'foo' diff --git a/packages/react/src/__tests__/ReactES6Class-test.js b/packages/react/src/__tests__/ReactES6Class-test.js index de9eab6399f9..e7226dc0bee4 100644 --- a/packages/react/src/__tests__/ReactES6Class-test.js +++ b/packages/react/src/__tests__/ReactES6Class-test.js @@ -602,7 +602,7 @@ describe('ReactES6Class', () => { 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' + - ' in Foo (at **)', + ' in Inner (at **)', ]); expect(ref.current.refs.inner.getName()).toBe('foo'); }); diff --git a/packages/react/src/__tests__/ReactElementClone-test.js b/packages/react/src/__tests__/ReactElementClone-test.js index 8dd9fceebc9c..11bb847518e4 100644 --- a/packages/react/src/__tests__/ReactElementClone-test.js +++ b/packages/react/src/__tests__/ReactElementClone-test.js @@ -359,16 +359,20 @@ describe('ReactElementClone', () => { const clone = React.cloneElement(element, props); expect(clone.type).toBe(ComponentClass); expect(clone.key).toBe('12'); - if (gate(flags => flags.enableRefAsProp)) { + if (gate(flags => flags.enableRefAsProp && flags.disableStringRefs)) { expect(clone.props.ref).toBe('34'); expect(() => expect(clone.ref).toBe('34')).toErrorDev( 'Accessing element.ref was removed in React 19', {withoutStack: true}, ); expect(clone.props).toEqual({foo: 'ef', ref: '34'}); - } else { - expect(clone.ref).toBe('34'); + } else if ( + gate(flags => !flags.enableRefAsProp && !flags.disableStringRefs) + ) { + expect(clone.ref).toBe(element.ref); expect(clone.props).toEqual({foo: 'ef'}); + } else { + // Not going to bother testing every possible combination. } if (__DEV__) { expect(Object.isFrozen(element)).toBe(true); diff --git a/packages/react/src/__tests__/ReactStrictMode-test.js b/packages/react/src/__tests__/ReactStrictMode-test.js index 588b65f139eb..eabf2a7bd59b 100644 --- a/packages/react/src/__tests__/ReactStrictMode-test.js +++ b/packages/react/src/__tests__/ReactStrictMode-test.js @@ -997,11 +997,11 @@ describe('string refs', () => { root.render(); }); }).toErrorDev( - 'Warning: Component "StrictMode" contains the string ref "somestring". ' + + 'Warning: Component "OuterComponent" contains the string ref "somestring". ' + 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' + - ' in OuterComponent (at **)', + ' in InnerComponent (at **)', ); await act(() => { @@ -1036,11 +1036,11 @@ describe('string refs', () => { root.render(); }); }).toErrorDev( - 'Warning: Component "StrictMode" contains the string ref "somestring". ' + + 'Warning: Component "OuterComponent" contains the string ref "somestring". ' + 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' + - ' in OuterComponent (at **)', + ' in InnerComponent (at **)', ); await act(() => { diff --git a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts index fa59b6ce644e..5a6e81bccf32 100644 --- a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts +++ b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts @@ -704,7 +704,7 @@ describe('ReactTypeScriptClass', function() { 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' + - ' in ClassicRefs (at **)', + ' in Inner (at **)', ]); expect(ref.current.refs.inner.getName()).toBe('foo'); }); diff --git a/packages/react/src/jsx/ReactJSXElement.js b/packages/react/src/jsx/ReactJSXElement.js index d20c1d4a25b5..6727626b4e5d 100644 --- a/packages/react/src/jsx/ReactJSXElement.js +++ b/packages/react/src/jsx/ReactJSXElement.js @@ -23,6 +23,9 @@ import { disableStringRefs, disableDefaultPropsExceptForClasses, } from 'shared/ReactFeatureFlags'; +import {checkPropStringCoercion} from 'shared/CheckStringCoercion'; +import {ClassComponent} from 'react-reconciler/src/ReactWorkTags'; +import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber'; const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner; const ReactDebugCurrentFrame = ReactSharedInternals.ReactDebugCurrentFrame; @@ -342,6 +345,9 @@ export function jsxProd(type, config, maybeKey) { if (hasValidRef(config)) { if (!enableRefAsProp) { ref = config.ref; + if (!disableStringRefs) { + ref = coerceStringRef(ref, ReactCurrentOwner.current, type); + } } } @@ -353,7 +359,15 @@ export function jsxProd(type, config, maybeKey) { propName !== 'key' && (enableRefAsProp || propName !== 'ref') ) { - props[propName] = config[propName]; + if (enableRefAsProp && !disableStringRefs && propName === 'ref') { + props.ref = coerceStringRef( + config[propName], + ReactCurrentOwner.current, + type, + ); + } else { + props[propName] = config[propName]; + } } } @@ -555,6 +569,9 @@ export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) { if (hasValidRef(config)) { if (!enableRefAsProp) { ref = config.ref; + if (!disableStringRefs) { + ref = coerceStringRef(ref, ReactCurrentOwner.current, type); + } } if (!disableStringRefs) { warnIfStringRefCannotBeAutoConverted(config, self); @@ -569,7 +586,15 @@ export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) { propName !== 'key' && (enableRefAsProp || propName !== 'ref') ) { - props[propName] = config[propName]; + if (enableRefAsProp && !disableStringRefs && propName === 'ref') { + props.ref = coerceStringRef( + config[propName], + ReactCurrentOwner.current, + type, + ); + } else { + props[propName] = config[propName]; + } } } @@ -687,6 +712,9 @@ export function createElement(type, config, children) { if (hasValidRef(config)) { if (!enableRefAsProp) { ref = config.ref; + if (!disableStringRefs) { + ref = coerceStringRef(ref, ReactCurrentOwner.current, type); + } } if (__DEV__ && !disableStringRefs) { @@ -714,7 +742,15 @@ export function createElement(type, config, children) { propName !== '__self' && propName !== '__source' ) { - props[propName] = config[propName]; + if (enableRefAsProp && !disableStringRefs && propName === 'ref') { + props.ref = coerceStringRef( + config[propName], + ReactCurrentOwner.current, + type, + ); + } else { + props[propName] = config[propName]; + } } } } @@ -820,6 +856,9 @@ export function cloneElement(element, config, children) { if (!enableRefAsProp) { // Silently steal the ref from the parent. ref = config.ref; + if (!disableStringRefs) { + ref = coerceStringRef(ref, owner, element.type); + } } owner = ReactCurrentOwner.current; } @@ -866,7 +905,11 @@ export function cloneElement(element, config, children) { // Resolve default props props[propName] = defaultProps[propName]; } else { - props[propName] = config[propName]; + if (enableRefAsProp && !disableStringRefs && propName === 'ref') { + props.ref = coerceStringRef(config[propName], owner, element.type); + } else { + props[propName] = config[propName]; + } } } } @@ -1086,3 +1129,89 @@ function validateFragmentProps(fragment) { } } } + +function coerceStringRef(mixedRef, owner, type) { + if (disableStringRefs) { + return mixedRef; + } + + let stringRef; + if (typeof mixedRef === 'string') { + stringRef = mixedRef; + } else { + if (typeof mixedRef === 'number' || typeof mixedRef === 'boolean') { + if (__DEV__) { + checkPropStringCoercion(mixedRef, 'ref'); + } + stringRef = '' + mixedRef; + } else { + return mixedRef; + } + } + + return stringRefAsCallbackRef.bind(null, stringRef, type, owner); +} + +function stringRefAsCallbackRef(stringRef, type, owner, value) { + if (disableStringRefs) { + return; + } + if (!owner) { + throw new Error( + `Element ref was specified as a string (${stringRef}) but no owner was set. This could happen for one of` + + ' the following reasons:\n' + + '1. You may be adding a ref to a function component\n' + + "2. You may be adding a ref to a component that was not created inside a component's render method\n" + + '3. You have multiple copies of React loaded\n' + + 'See https://react.dev/link/refs-must-have-owner for more information.', + ); + } + if (owner.tag !== ClassComponent) { + throw new Error( + 'Function components cannot have string refs. ' + + 'We recommend using useRef() instead. ' + + 'Learn more about using refs safely here: ' + + 'https://react.dev/link/strict-mode-string-ref', + ); + } + + if (__DEV__) { + if ( + // Will already warn with "Function components cannot be given refs" + !(typeof type === 'function' && !isReactClass(type)) + ) { + const componentName = getComponentNameFromFiber(owner) || 'Component'; + if (!didWarnAboutStringRefs[componentName]) { + console.error( + 'Component "%s" contains the string ref "%s". Support for string refs ' + + 'will be removed in a future major release. We recommend using ' + + 'useRef() or createRef() instead. ' + + 'Learn more about using refs safely here: ' + + 'https://react.dev/link/strict-mode-string-ref', + componentName, + stringRef, + ); + didWarnAboutStringRefs[componentName] = true; + } + } + } + + const inst = owner.stateNode; + if (!inst) { + throw new Error( + `Missing owner for string ref ${stringRef}. This error is likely caused by a ` + + 'bug in React. Please file an issue.', + ); + } + + const refs = inst.refs; + if (value === null) { + delete refs[stringRef]; + } else { + refs[stringRef] = value; + } +} + +function isReactClass(type) { + return type.prototype && type.prototype.isReactComponent; +} From f33a6b69c6cb406ea0cc51d07bc4d3fd2d8d8744 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 5 Apr 2024 12:48:52 -0400 Subject: [PATCH 2/4] Track Owner for Server Components in DEV (#28753) This implements the concept of a DEV-only "owner" for Server Components. The owner concept isn't really super useful. We barely use it anymore, but we do have it as a concept in DevTools in a couple of cases so this adds it for parity. However, this is mainly interesting because it could be used to wire up future owner-based stacks. I do this by outlining the DebugInfo for a Server Component (ReactComponentInfo). Then I just rely on Flight deduping to refer to that. I refer to the same thing by referential equality so that we can associate a Server Component parent in DebugInfo with an owner. If you suspend and replay a Server Component, we have to restore the same owner. To do that, I did a little ugly hack and stashed it on the thenable state object. Felt unnecessarily complicated to add a stateful wrapper for this one dev-only case. The owner could really be anything since it could be coming from a different implementation. Because this is the first time we have an owner other than Fiber, I have to fix up a bunch of places that assumes Fiber. I mainly did the `typeof owner.tag === 'number'` to assume it's a Fiber for now. This also doesn't actually add it to DevTools / RN Inspector yet. I just ignore them there for now. Because Server Components can be async the owner isn't tracked after an await. We need per-component AsyncLocalStorage for that. This can be done in a follow up. --- .../react-client/src/ReactFlightClient.js | 25 +++-- .../src/__tests__/ReactFlight-test.js | 58 +++++++++- .../src/__tests__/componentStacks-test.js | 1 + .../backend/DevToolsComponentStackFrame.js | 22 +--- .../backend/DevToolsFiberComponentStack.js | 16 +-- .../src/backend/renderer.js | 53 ++++++--- .../src/ReactNativeFiberInspector.js | 26 +++-- .../react-reconciler/src/ReactCurrentFiber.js | 6 +- packages/react-reconciler/src/ReactFiber.js | 7 +- .../src/ReactFiberComponentStack.js | 19 ++-- .../src/ReactInternalTypes.js | 3 +- .../src/getComponentNameFromFiber.js | 13 +++ .../src/__tests__/ReactFlightDOMEdge-test.js | 2 +- .../src/ReactFizzComponentStack.js | 6 +- packages/react-server/src/ReactFlightHooks.js | 13 ++- .../react-server/src/ReactFlightServer.js | 103 +++++++++++++++--- .../react/src/__tests__/ReactFetch-test.js | 2 +- packages/react/src/jsx/ReactJSXElement.js | 12 +- packages/shared/ReactComponentStackFrame.js | 61 +++-------- packages/shared/ReactTypes.js | 1 + 20 files changed, 291 insertions(+), 158 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 082f94261bfd..c0cec0db366c 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -484,6 +484,7 @@ function createElement( type: mixed, key: mixed, props: mixed, + owner: null | ReactComponentInfo, // DEV-only ): React$Element { let element: any; if (__DEV__ && enableRefAsProp) { @@ -493,7 +494,7 @@ function createElement( type, key, props, - _owner: null, + _owner: owner, }: any); Object.defineProperty(element, 'ref', { enumerable: false, @@ -520,7 +521,7 @@ function createElement( props, // Record the component responsible for creating this element. - _owner: null, + _owner: owner, }: any); } @@ -854,7 +855,12 @@ function parseModelTuple( if (tuple[0] === REACT_ELEMENT_TYPE) { // TODO: Consider having React just directly accept these arrays as elements. // Or even change the ReactElement type to be an array. - return createElement(tuple[1], tuple[2], tuple[3]); + return createElement( + tuple[1], + tuple[2], + tuple[3], + __DEV__ ? (tuple: any)[4] : null, + ); } return value; } @@ -1132,12 +1138,14 @@ function resolveConsoleEntry( ); } - const payload: [string, string, string, mixed] = parseModel(response, value); + const payload: [string, string, null | ReactComponentInfo, string, mixed] = + parseModel(response, value); const methodName = payload[0]; // TODO: Restore the fake stack before logging. // const stackTrace = payload[1]; - const env = payload[2]; - const args = payload.slice(3); + // const owner = payload[2]; + const env = payload[3]; + const args = payload.slice(4); printToConsole(methodName, args, env); } @@ -1286,7 +1294,10 @@ function processFullRow( } case 68 /* "D" */: { if (__DEV__) { - const debugInfo = JSON.parse(row); + const debugInfo: ReactComponentInfo | ReactAsyncInfo = parseModel( + response, + row, + ); resolveDebugInfo(response, id, debugInfo); return; } diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index 55956ddc93ec..6fbd2360c82e 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -214,7 +214,7 @@ describe('ReactFlight', () => { const rootModel = await ReactNoopFlightClient.read(transport); const greeting = rootModel.greeting; expect(greeting._debugInfo).toEqual( - __DEV__ ? [{name: 'Greeting', env: 'Server'}] : undefined, + __DEV__ ? [{name: 'Greeting', env: 'Server', owner: null}] : undefined, ); ReactNoop.render(greeting); }); @@ -241,7 +241,7 @@ describe('ReactFlight', () => { await act(async () => { const promise = ReactNoopFlightClient.read(transport); expect(promise._debugInfo).toEqual( - __DEV__ ? [{name: 'Greeting', env: 'Server'}] : undefined, + __DEV__ ? [{name: 'Greeting', env: 'Server', owner: null}] : undefined, ); ReactNoop.render(await promise); }); @@ -2072,19 +2072,21 @@ describe('ReactFlight', () => { await act(async () => { const promise = ReactNoopFlightClient.read(transport); expect(promise._debugInfo).toEqual( - __DEV__ ? [{name: 'ServerComponent', env: 'Server'}] : undefined, + __DEV__ + ? [{name: 'ServerComponent', env: 'Server', owner: null}] + : undefined, ); const result = await promise; const thirdPartyChildren = await result.props.children[1]; // We expect the debug info to be transferred from the inner stream to the outer. expect(thirdPartyChildren[0]._debugInfo).toEqual( __DEV__ - ? [{name: 'ThirdPartyComponent', env: 'third-party'}] + ? [{name: 'ThirdPartyComponent', env: 'third-party', owner: null}] : undefined, ); expect(thirdPartyChildren[1]._debugInfo).toEqual( __DEV__ - ? [{name: 'ThirdPartyLazyComponent', env: 'third-party'}] + ? [{name: 'ThirdPartyLazyComponent', env: 'third-party', owner: null}] : undefined, ); ReactNoop.render(result); @@ -2145,4 +2147,50 @@ describe('ReactFlight', () => { expect(loggedFn).not.toBe(foo); expect(loggedFn.toString()).toBe(foo.toString()); }); + + it('uses the server component debug info as the element owner in DEV', async () => { + function Container({children}) { + return children; + } + + function Greeting({firstName}) { + // We can't use JSX here because it'll use the Client React. + return ReactServer.createElement( + Container, + null, + ReactServer.createElement('span', null, 'Hello, ', firstName), + ); + } + + const model = { + greeting: ReactServer.createElement(Greeting, {firstName: 'Seb'}), + }; + + const transport = ReactNoopFlightServer.render(model); + + await act(async () => { + const rootModel = await ReactNoopFlightClient.read(transport); + const greeting = rootModel.greeting; + // We've rendered down to the span. + expect(greeting.type).toBe('span'); + if (__DEV__) { + const greetInfo = {name: 'Greeting', env: 'Server', owner: null}; + expect(greeting._debugInfo).toEqual([ + greetInfo, + {name: 'Container', env: 'Server', owner: greetInfo}, + ]); + // The owner that created the span was the outer server component. + // We expect the debug info to be referentially equal to the owner. + expect(greeting._owner).toBe(greeting._debugInfo[0]); + } else { + expect(greeting._debugInfo).toBe(undefined); + expect(greeting._owner).toBe( + gate(flags => flags.disableStringRefs) ? undefined : null, + ); + } + ReactNoop.render(greeting); + }); + + expect(ReactNoop).toMatchRenderedOutput(Hello, Seb); + }); }); diff --git a/packages/react-devtools-shared/src/__tests__/componentStacks-test.js b/packages/react-devtools-shared/src/__tests__/componentStacks-test.js index 3d17546e39d1..02e2ad6802f6 100644 --- a/packages/react-devtools-shared/src/__tests__/componentStacks-test.js +++ b/packages/react-devtools-shared/src/__tests__/componentStacks-test.js @@ -101,6 +101,7 @@ describe('component stack', () => { { name: 'ServerComponent', env: 'Server', + owner: null, }, ]; const Parent = () => ChildPromise; diff --git a/packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js b/packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js index 0e412a43f74c..316245492c64 100644 --- a/packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js +++ b/packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js @@ -33,10 +33,7 @@ import { import {disableLogs, reenableLogs} from './DevToolsConsolePatching'; let prefix; -export function describeBuiltInComponentFrame( - name: string, - ownerFn: void | null | Function, -): string { +export function describeBuiltInComponentFrame(name: string): string { if (prefix === undefined) { // Extract the VM specific prefix used by each line. try { @@ -51,10 +48,7 @@ export function describeBuiltInComponentFrame( } export function describeDebugInfoFrame(name: string, env: ?string): string { - return describeBuiltInComponentFrame( - name + (env ? ' (' + env + ')' : ''), - null, - ); + return describeBuiltInComponentFrame(name + (env ? ' (' + env + ')' : '')); } let reentry = false; @@ -292,7 +286,6 @@ export function describeNativeComponentFrame( export function describeClassComponentFrame( ctor: Function, - ownerFn: void | null | Function, currentDispatcherRef: CurrentDispatcherRef, ): string { return describeNativeComponentFrame(ctor, true, currentDispatcherRef); @@ -300,7 +293,6 @@ export function describeClassComponentFrame( export function describeFunctionComponentFrame( fn: Function, - ownerFn: void | null | Function, currentDispatcherRef: CurrentDispatcherRef, ): string { return describeNativeComponentFrame(fn, false, currentDispatcherRef); @@ -313,7 +305,6 @@ function shouldConstruct(Component: Function) { export function describeUnknownElementTypeFrameInDEV( type: any, - ownerFn: void | null | Function, currentDispatcherRef: CurrentDispatcherRef, ): string { if (!__DEV__) { @@ -330,15 +321,15 @@ export function describeUnknownElementTypeFrameInDEV( ); } if (typeof type === 'string') { - return describeBuiltInComponentFrame(type, ownerFn); + return describeBuiltInComponentFrame(type); } switch (type) { case SUSPENSE_NUMBER: case SUSPENSE_SYMBOL_STRING: - return describeBuiltInComponentFrame('Suspense', ownerFn); + return describeBuiltInComponentFrame('Suspense'); case SUSPENSE_LIST_NUMBER: case SUSPENSE_LIST_SYMBOL_STRING: - return describeBuiltInComponentFrame('SuspenseList', ownerFn); + return describeBuiltInComponentFrame('SuspenseList'); } if (typeof type === 'object') { switch (type.$$typeof) { @@ -346,7 +337,6 @@ export function describeUnknownElementTypeFrameInDEV( case FORWARD_REF_SYMBOL_STRING: return describeFunctionComponentFrame( type.render, - ownerFn, currentDispatcherRef, ); case MEMO_NUMBER: @@ -354,7 +344,6 @@ export function describeUnknownElementTypeFrameInDEV( // Memo may contain any component type so we recursively resolve it. return describeUnknownElementTypeFrameInDEV( type.type, - ownerFn, currentDispatcherRef, ); case LAZY_NUMBER: @@ -366,7 +355,6 @@ export function describeUnknownElementTypeFrameInDEV( // Lazy may contain any component type so we recursively resolve it. return describeUnknownElementTypeFrameInDEV( init(payload), - ownerFn, currentDispatcherRef, ); } catch (x) {} diff --git a/packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js b/packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js index 77f0adf37ea4..6d99cf4f21e5 100644 --- a/packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js +++ b/packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js @@ -39,38 +39,30 @@ export function describeFiber( ClassComponent, } = workTagMap; - const owner: null | Function = __DEV__ - ? workInProgress._debugOwner - ? workInProgress._debugOwner.type - : null - : null; switch (workInProgress.tag) { case HostComponent: - return describeBuiltInComponentFrame(workInProgress.type, owner); + return describeBuiltInComponentFrame(workInProgress.type); case LazyComponent: - return describeBuiltInComponentFrame('Lazy', owner); + return describeBuiltInComponentFrame('Lazy'); case SuspenseComponent: - return describeBuiltInComponentFrame('Suspense', owner); + return describeBuiltInComponentFrame('Suspense'); case SuspenseListComponent: - return describeBuiltInComponentFrame('SuspenseList', owner); + return describeBuiltInComponentFrame('SuspenseList'); case FunctionComponent: case IndeterminateComponent: case SimpleMemoComponent: return describeFunctionComponentFrame( workInProgress.type, - owner, currentDispatcherRef, ); case ForwardRef: return describeFunctionComponentFrame( workInProgress.type.render, - owner, currentDispatcherRef, ); case ClassComponent: return describeClassComponentFrame( workInProgress.type, - owner, currentDispatcherRef, ); default: diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 803ef4b76b54..1f333d1271d6 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -1952,15 +1952,24 @@ export function attach( const {key} = fiber; const displayName = getDisplayNameForFiber(fiber); const elementType = getElementTypeForFiber(fiber); - const {_debugOwner} = fiber; + const debugOwner = fiber._debugOwner; // Ideally we should call getFiberIDThrows() for _debugOwner, // since owners are almost always higher in the tree (and so have already been processed), // but in some (rare) instances reported in open source, a descendant mounts before an owner. // Since this is a DEV only field it's probably okay to also just lazily generate and ID here if needed. // See https://github.com/facebook/react/issues/21445 - const ownerID = - _debugOwner != null ? getOrGenerateFiberID(_debugOwner) : 0; + let ownerID: number; + if (debugOwner != null) { + if (typeof debugOwner.tag === 'number') { + ownerID = getOrGenerateFiberID((debugOwner: any)); + } else { + // TODO: Track Server Component Owners. + ownerID = 0; + } + } else { + ownerID = 0; + } const parentID = parentFiber ? getFiberIDThrows(parentFiber) : 0; const displayNameStringID = getStringID(displayName); @@ -3104,15 +3113,17 @@ export function attach( return null; } - const {_debugOwner} = fiber; - const owners: Array = [fiberToSerializedElement(fiber)]; - if (_debugOwner) { - let owner: null | Fiber = _debugOwner; - while (owner !== null) { - owners.unshift(fiberToSerializedElement(owner)); - owner = owner._debugOwner || null; + let owner = fiber._debugOwner; + while (owner != null) { + if (typeof owner.tag === 'number') { + const ownerFiber: Fiber = (owner: any); // Refined + owners.unshift(fiberToSerializedElement(ownerFiber)); + owner = ownerFiber._debugOwner; + } else { + // TODO: Track Server Component Owners. + break; } } @@ -3173,7 +3184,7 @@ export function attach( } const { - _debugOwner, + _debugOwner: debugOwner, stateNode, key, memoizedProps, @@ -3300,13 +3311,19 @@ export function attach( context = {value: context}; } - let owners = null; - if (_debugOwner) { - owners = ([]: Array); - let owner: null | Fiber = _debugOwner; - while (owner !== null) { - owners.push(fiberToSerializedElement(owner)); - owner = owner._debugOwner || null; + let owners: null | Array = null; + let owner = debugOwner; + while (owner != null) { + if (typeof owner.tag === 'number') { + const ownerFiber: Fiber = (owner: any); // Refined + if (owners === null) { + owners = []; + } + owners.push(fiberToSerializedElement(ownerFiber)); + owner = ownerFiber._debugOwner; + } else { + // TODO: Track Server Component Owners. + break; } } diff --git a/packages/react-native-renderer/src/ReactNativeFiberInspector.js b/packages/react-native-renderer/src/ReactNativeFiberInspector.js index f30012b2cf91..14d87f7a5502 100644 --- a/packages/react-native-renderer/src/ReactNativeFiberInspector.js +++ b/packages/react-native-renderer/src/ReactNativeFiberInspector.js @@ -103,13 +103,21 @@ function getInspectorDataForInstance( } const fiber = findCurrentFiberUsingSlowPath(closestInstance); + if (fiber === null) { + // Might not be currently mounted. + return { + hierarchy: [], + props: emptyObject, + selectedIndex: null, + componentStack: '', + }; + } const fiberHierarchy = getOwnerHierarchy(fiber); const instance = lastNonHostInstance(fiberHierarchy); const hierarchy = createHierarchy(fiberHierarchy); const props = getHostProps(instance); const selectedIndex = fiberHierarchy.indexOf(instance); - const componentStack = - fiber !== null ? getStackByFiberInDevAndProd(fiber) : ''; + const componentStack = getStackByFiberInDevAndProd(fiber); return { closestInstance: instance, @@ -125,7 +133,7 @@ function getInspectorDataForInstance( ); } -function getOwnerHierarchy(instance: any) { +function getOwnerHierarchy(instance: Fiber) { const hierarchy: Array<$FlowFixMe> = []; traverseOwnerTreeUp(hierarchy, instance); return hierarchy; @@ -143,15 +151,17 @@ function lastNonHostInstance(hierarchy) { return hierarchy[0]; } -// $FlowFixMe[missing-local-annot] function traverseOwnerTreeUp( hierarchy: Array<$FlowFixMe>, - instance: any, + instance: Fiber, ): void { if (__DEV__ || enableGetInspectorDataForInstanceInProduction) { - if (instance) { - hierarchy.unshift(instance); - traverseOwnerTreeUp(hierarchy, instance._debugOwner); + hierarchy.unshift(instance); + const owner = instance._debugOwner; + if (owner != null && typeof owner.tag === 'number') { + traverseOwnerTreeUp(hierarchy, (owner: any)); + } else { + // TODO: Traverse Server Components owners. } } } diff --git a/packages/react-reconciler/src/ReactCurrentFiber.js b/packages/react-reconciler/src/ReactCurrentFiber.js index 6c283ed0d35f..c52d55c0cce7 100644 --- a/packages/react-reconciler/src/ReactCurrentFiber.js +++ b/packages/react-reconciler/src/ReactCurrentFiber.js @@ -11,7 +11,7 @@ import type {Fiber} from './ReactInternalTypes'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import {getStackByFiberInDevAndProd} from './ReactFiberComponentStack'; -import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber'; +import {getComponentNameFromOwner} from 'react-reconciler/src/getComponentNameFromFiber'; const ReactDebugCurrentFrame = ReactSharedInternals.ReactDebugCurrentFrame; @@ -24,8 +24,8 @@ export function getCurrentFiberOwnerNameInDevOrNull(): string | null { return null; } const owner = current._debugOwner; - if (owner !== null && typeof owner !== 'undefined') { - return getComponentNameFromFiber(owner); + if (owner != null) { + return getComponentNameFromOwner(owner); } } return null; diff --git a/packages/react-reconciler/src/ReactFiber.js b/packages/react-reconciler/src/ReactFiber.js index 7b1a6514ebd5..1de7d42cfe7d 100644 --- a/packages/react-reconciler/src/ReactFiber.js +++ b/packages/react-reconciler/src/ReactFiber.js @@ -68,7 +68,7 @@ import { TracingMarkerComponent, } from './ReactWorkTags'; import {OffscreenVisible} from './ReactFiberActivityComponent'; -import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber'; +import {getComponentNameFromOwner} from 'react-reconciler/src/getComponentNameFromFiber'; import {isDevToolsPresent} from './ReactFiberDevToolsHook'; import { resolveClassForHotReloading, @@ -110,6 +110,7 @@ import { attachOffscreenInstance, } from './ReactFiberCommitWork'; import {getHostContext} from './ReactFiberHostContext'; +import type {ReactComponentInfo} from '../../shared/ReactTypes'; export type {Fiber}; @@ -475,7 +476,7 @@ export function createFiberFromTypeAndProps( type: any, // React$ElementType key: null | string, pendingProps: any, - owner: null | Fiber, + owner: null | ReactComponentInfo | Fiber, mode: TypeOfMode, lanes: Lanes, ): Fiber { @@ -610,7 +611,7 @@ export function createFiberFromTypeAndProps( "it's defined in, or you might have mixed up default and " + 'named imports.'; } - const ownerName = owner ? getComponentNameFromFiber(owner) : null; + const ownerName = owner ? getComponentNameFromOwner(owner) : null; if (ownerName) { info += '\n\nCheck the render method of `' + ownerName + '`.'; } diff --git a/packages/react-reconciler/src/ReactFiberComponentStack.js b/packages/react-reconciler/src/ReactFiberComponentStack.js index f292cb51d10b..193c27aef1e1 100644 --- a/packages/react-reconciler/src/ReactFiberComponentStack.js +++ b/packages/react-reconciler/src/ReactFiberComponentStack.js @@ -29,29 +29,24 @@ import { } from 'shared/ReactComponentStackFrame'; function describeFiber(fiber: Fiber): string { - const owner: null | Function = __DEV__ - ? fiber._debugOwner - ? fiber._debugOwner.type - : null - : null; switch (fiber.tag) { case HostHoistable: case HostSingleton: case HostComponent: - return describeBuiltInComponentFrame(fiber.type, owner); + return describeBuiltInComponentFrame(fiber.type); case LazyComponent: - return describeBuiltInComponentFrame('Lazy', owner); + return describeBuiltInComponentFrame('Lazy'); case SuspenseComponent: - return describeBuiltInComponentFrame('Suspense', owner); + return describeBuiltInComponentFrame('Suspense'); case SuspenseListComponent: - return describeBuiltInComponentFrame('SuspenseList', owner); + return describeBuiltInComponentFrame('SuspenseList'); case FunctionComponent: case SimpleMemoComponent: - return describeFunctionComponentFrame(fiber.type, owner); + return describeFunctionComponentFrame(fiber.type); case ForwardRef: - return describeFunctionComponentFrame(fiber.type.render, owner); + return describeFunctionComponentFrame(fiber.type.render); case ClassComponent: - return describeClassComponentFrame(fiber.type, owner); + return describeClassComponentFrame(fiber.type); default: return ''; } diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index 3b82dc40c828..f12b9a16c570 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -15,6 +15,7 @@ import type { Usable, ReactFormState, Awaited, + ReactComponentInfo, ReactDebugInfo, } from 'shared/ReactTypes'; import type {WorkTag} from './ReactWorkTags'; @@ -193,7 +194,7 @@ export type Fiber = { // __DEV__ only _debugInfo?: ReactDebugInfo | null, - _debugOwner?: Fiber | null, + _debugOwner?: ReactComponentInfo | Fiber | null, _debugIsCurrentlyTiming?: boolean, _debugNeedsRemount?: boolean, diff --git a/packages/react-reconciler/src/getComponentNameFromFiber.js b/packages/react-reconciler/src/getComponentNameFromFiber.js index 6659b14b7f0d..332324900fa8 100644 --- a/packages/react-reconciler/src/getComponentNameFromFiber.js +++ b/packages/react-reconciler/src/getComponentNameFromFiber.js @@ -47,6 +47,7 @@ import { } from 'react-reconciler/src/ReactWorkTags'; import getComponentNameFromType from 'shared/getComponentNameFromType'; import {REACT_STRICT_MODE_TYPE} from 'shared/ReactSymbols'; +import type {ReactComponentInfo} from '../../shared/ReactTypes'; // Keep in sync with shared/getComponentNameFromType function getWrappedName( @@ -66,6 +67,18 @@ function getContextName(type: ReactContext) { return type.displayName || 'Context'; } +export function getComponentNameFromOwner( + owner: Fiber | ReactComponentInfo, +): string | null { + if (typeof owner.tag === 'number') { + return getComponentNameFromFiber((owner: any)); + } + if (typeof owner.name === 'string') { + return owner.name; + } + return null; +} + export default function getComponentNameFromFiber(fiber: Fiber): string | null { const {tag, type} = fiber; switch (tag) { diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js index cce874d8148d..8304d9927d37 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js @@ -290,7 +290,7 @@ describe('ReactFlightDOMEdge', () => { , ); const serializedContent = await readResult(stream); - const expectedDebugInfoSize = __DEV__ ? 42 * 20 : 0; + const expectedDebugInfoSize = __DEV__ ? 64 * 20 : 0; expect(serializedContent.length).toBeLessThan(150 + expectedDebugInfoSize); }); diff --git a/packages/react-server/src/ReactFizzComponentStack.js b/packages/react-server/src/ReactFizzComponentStack.js index 7aeb6b0802c9..84b4d82d8d45 100644 --- a/packages/react-server/src/ReactFizzComponentStack.js +++ b/packages/react-server/src/ReactFizzComponentStack.js @@ -43,13 +43,13 @@ export function getStackByComponentStackNode( do { switch (node.tag) { case 0: - info += describeBuiltInComponentFrame(node.type, null); + info += describeBuiltInComponentFrame(node.type); break; case 1: - info += describeFunctionComponentFrame(node.type, null); + info += describeFunctionComponentFrame(node.type); break; case 2: - info += describeClassComponentFrame(node.type, null); + info += describeClassComponentFrame(node.type); break; } // $FlowFixMe[incompatible-type] we bail out when we get a null diff --git a/packages/react-server/src/ReactFlightHooks.js b/packages/react-server/src/ReactFlightHooks.js index 75a99dc558ea..e85e0e9ce6a6 100644 --- a/packages/react-server/src/ReactFlightHooks.js +++ b/packages/react-server/src/ReactFlightHooks.js @@ -9,7 +9,7 @@ import type {Dispatcher} from 'react-reconciler/src/ReactInternalTypes'; import type {Request} from './ReactFlightServer'; -import type {Thenable, Usable} from 'shared/ReactTypes'; +import type {Thenable, Usable, ReactComponentInfo} from 'shared/ReactTypes'; import type {ThenableState} from './ReactFlightThenable'; import { REACT_MEMO_CACHE_SENTINEL, @@ -21,6 +21,7 @@ import {isClientReference} from './ReactFlightServerConfig'; let currentRequest = null; let thenableIndexCounter = 0; let thenableState = null; +let currentComponentDebugInfo = null; export function prepareToUseHooksForRequest(request: Request) { currentRequest = request; @@ -32,9 +33,13 @@ export function resetHooksForRequest() { export function prepareToUseHooksForComponent( prevThenableState: ThenableState | null, + componentDebugInfo: null | ReactComponentInfo, ) { thenableIndexCounter = 0; thenableState = prevThenableState; + if (__DEV__) { + currentComponentDebugInfo = componentDebugInfo; + } } export function getThenableStateAfterSuspending(): ThenableState { @@ -42,6 +47,12 @@ export function getThenableStateAfterSuspending(): ThenableState { // which is not really supported anymore, it will be empty. We use the empty set as a // marker to know if this was a replay of the same component or first attempt. const state = thenableState || createThenableState(); + if (__DEV__) { + // This is a hack but we stash the debug info here so that we don't need a completely + // different data structure just for this in DEV. Not too happy about it. + (state: any)._componentDebugInfo = currentComponentDebugInfo; + currentComponentDebugInfo = null; + } thenableState = null; return state; } diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index ad0a28f37175..a8c648881fe2 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -58,6 +58,7 @@ import type { ReactComponentInfo, ReactAsyncInfo, } from 'shared/ReactTypes'; +import type {ReactElement} from 'shared/ReactElementType'; import type {LazyComponent} from 'react/src/ReactLazy'; import type {TemporaryReference} from './ReactFlightServerTemporaryReferences'; @@ -153,7 +154,8 @@ function patchConsole(consoleInst: typeof console, methodName: string) { // We don't currently use this id for anything but we emit it so that we can later // refer to previous logs in debug info to associate them with a component. const id = request.nextChunkId++; - emitConsoleChunk(request, id, methodName, stack, arguments); + const owner: null | ReactComponentInfo = ReactCurrentOwner.current; + emitConsoleChunk(request, id, methodName, owner, stack, arguments); } // $FlowFixMe[prop-missing] return originalMethod.apply(this, arguments); @@ -303,6 +305,7 @@ const { ReactCurrentCache, } = ReactServerSharedInternals; const ReactCurrentDispatcher = ReactSharedInternals.ReactCurrentDispatcher; +const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner; function throwTaintViolation(message: string) { // eslint-disable-next-line react-internal/prod-error-codes @@ -594,6 +597,7 @@ function renderFunctionComponent( key: null | string, Component: (p: Props, arg: void) => any, props: Props, + owner: null | ReactComponentInfo, ): ReactJSONValue { // Reset the task's thenable state before continuing, so that if a later // component suspends we can reuse the same task object. If the same @@ -601,6 +605,7 @@ function renderFunctionComponent( const prevThenableState = task.thenableState; task.thenableState = null; + let componentDebugInfo: null | ReactComponentInfo = null; if (__DEV__) { if (debugID === null) { // We don't have a chunk to assign debug info. We need to outline this @@ -609,22 +614,42 @@ function renderFunctionComponent( } else if (prevThenableState !== null) { // This is a replay and we've already emitted the debug info of this component // in the first pass. We skip emitting a duplicate line. + // As a hack we stashed the previous component debug info on this object in DEV. + componentDebugInfo = (prevThenableState: any)._componentDebugInfo; } else { // This is a new component in the same task so we can emit more debug info. const componentName = (Component: any).displayName || Component.name || ''; request.pendingChunks++; - emitDebugChunk(request, debugID, { + + const componentDebugID = debugID; + componentDebugInfo = { name: componentName, env: request.environmentName, - }); + owner: owner, + }; + // We outline this model eagerly so that we can refer to by reference as an owner. + // If we had a smarter way to dedupe we might not have to do this if there ends up + // being no references to this as an owner. + outlineModel(request, componentDebugInfo); + emitDebugChunk(request, componentDebugID, componentDebugInfo); } } - prepareToUseHooksForComponent(prevThenableState); + prepareToUseHooksForComponent(prevThenableState, componentDebugInfo); // The secondArg is always undefined in Server Components since refs error early. const secondArg = undefined; - let result = Component(props, secondArg); + let result; + if (__DEV__) { + ReactCurrentOwner.current = componentDebugInfo; + try { + result = Component(props, secondArg); + } finally { + ReactCurrentOwner.current = null; + } + } else { + result = Component(props, secondArg); + } if ( typeof result === 'object' && result !== null && @@ -723,9 +748,12 @@ function renderClientElement( type: any, key: null | string, props: any, + owner: null | ReactComponentInfo, // DEV-only ): ReactJSONValue { if (!enableServerComponentKeys) { - return [REACT_ELEMENT_TYPE, type, key, props]; + return __DEV__ + ? [REACT_ELEMENT_TYPE, type, key, props, owner] + : [REACT_ELEMENT_TYPE, type, key, props]; } // We prepend the terminal client element that actually gets serialized with // the keys of any Server Components which are not serialized. @@ -735,7 +763,9 @@ function renderClientElement( } else if (keyPath !== null) { key = keyPath + ',' + key; } - const element = [REACT_ELEMENT_TYPE, type, key, props]; + const element = __DEV__ + ? [REACT_ELEMENT_TYPE, type, key, props, owner] + : [REACT_ELEMENT_TYPE, type, key, props]; if (task.implicitSlot && key !== null) { // The root Server Component had no key so it was in an implicit slot. // If we had a key lower, it would end up in that slot with an explicit key. @@ -781,6 +811,7 @@ function renderElement( key: null | string, ref: mixed, props: any, + owner: null | ReactComponentInfo, // DEV only ): ReactJSONValue { if (ref !== null && ref !== undefined) { // When the ref moves to the regular props object this will implicitly @@ -801,13 +832,13 @@ function renderElement( if (typeof type === 'function') { if (isClientReference(type) || isTemporaryReference(type)) { // This is a reference to a Client Component. - return renderClientElement(task, type, key, props); + return renderClientElement(task, type, key, props, owner); } // This is a Server Component. - return renderFunctionComponent(request, task, key, type, props); + return renderFunctionComponent(request, task, key, type, props, owner); } else if (typeof type === 'string') { // This is a host element. E.g. HTML. - return renderClientElement(task, type, key, props); + return renderClientElement(task, type, key, props, owner); } else if (typeof type === 'symbol') { if (type === REACT_FRAGMENT_TYPE && key === null) { // For key-less fragments, we add a small optimization to avoid serializing @@ -828,24 +859,39 @@ function renderElement( } // This might be a built-in React component. We'll let the client decide. // Any built-in works as long as its props are serializable. - return renderClientElement(task, type, key, props); + return renderClientElement(task, type, key, props, owner); } else if (type != null && typeof type === 'object') { if (isClientReference(type)) { // This is a reference to a Client Component. - return renderClientElement(task, type, key, props); + return renderClientElement(task, type, key, props, owner); } switch (type.$$typeof) { case REACT_LAZY_TYPE: { const payload = type._payload; const init = type._init; const wrappedType = init(payload); - return renderElement(request, task, wrappedType, key, ref, props); + return renderElement( + request, + task, + wrappedType, + key, + ref, + props, + owner, + ); } case REACT_FORWARD_REF_TYPE: { - return renderFunctionComponent(request, task, key, type.render, props); + return renderFunctionComponent( + request, + task, + key, + type.render, + props, + owner, + ); } case REACT_MEMO_TYPE: { - return renderElement(request, task, type.type, key, ref, props); + return renderElement(request, task, type.type, key, ref, props, owner); } } } @@ -1356,7 +1402,7 @@ function renderModelDestructive( writtenObjects.set((value: any).props, NEVER_OUTLINED); } - const element: React$Element = (value: any); + const element: ReactElement = (value: any); if (__DEV__) { const debugInfo: ?ReactDebugInfo = (value: any)._debugInfo; @@ -1394,6 +1440,7 @@ function renderModelDestructive( element.key, ref, props, + __DEV__ ? element._owner : null, ); } case REACT_LAZY_TYPE: { @@ -1904,8 +1951,27 @@ function emitDebugChunk( ); } + // We use the console encoding so that we can dedupe objects but don't necessarily + // use the full serialization that requires a task. + const counter = {objectCount: 0}; + function replacer( + this: + | {+[key: string | number]: ReactClientValue} + | $ReadOnlyArray, + parentPropertyName: string, + value: ReactClientValue, + ): ReactJSONValue { + return renderConsoleValue( + request, + counter, + this, + parentPropertyName, + value, + ); + } + // $FlowFixMe[incompatible-type] stringify can return null - const json: string = stringify(debugInfo); + const json: string = stringify(debugInfo, replacer); const row = serializeRowHeader('D', id) + json + '\n'; const processedChunk = stringToChunk(row); request.completedRegularChunks.push(processedChunk); @@ -2207,6 +2273,7 @@ function emitConsoleChunk( request: Request, id: number, methodName: string, + owner: null | ReactComponentInfo, stackTrace: string, args: Array, ): void { @@ -2241,7 +2308,7 @@ function emitConsoleChunk( // TODO: Don't double badge if this log came from another Flight Client. const env = request.environmentName; - const payload = [methodName, stackTrace, env]; + const payload = [methodName, stackTrace, owner, env]; // $FlowFixMe[method-unbinding] payload.push.apply(payload, args); // $FlowFixMe[incompatible-type] stringify can return null diff --git a/packages/react/src/__tests__/ReactFetch-test.js b/packages/react/src/__tests__/ReactFetch-test.js index cd658066c8ca..e612bdf6a978 100644 --- a/packages/react/src/__tests__/ReactFetch-test.js +++ b/packages/react/src/__tests__/ReactFetch-test.js @@ -86,7 +86,7 @@ describe('ReactFetch', () => { const promise = render(Component); expect(await promise).toMatchInlineSnapshot(`"GET world []"`); expect(promise._debugInfo).toEqual( - __DEV__ ? [{name: 'Component', env: 'Server'}] : undefined, + __DEV__ ? [{name: 'Component', env: 'Server', owner: null}] : undefined, ); expect(fetchCount).toBe(1); }); diff --git a/packages/react/src/jsx/ReactJSXElement.js b/packages/react/src/jsx/ReactJSXElement.js index 6727626b4e5d..d52355b18468 100644 --- a/packages/react/src/jsx/ReactJSXElement.js +++ b/packages/react/src/jsx/ReactJSXElement.js @@ -1051,13 +1051,17 @@ function validateExplicitKey(element, parentType) { let childOwner = ''; if ( element && - element._owner && + element._owner != null && element._owner !== ReactCurrentOwner.current ) { + let ownerName = null; + if (typeof element._owner.tag === 'number') { + ownerName = getComponentNameFromType(element._owner.type); + } else if (typeof element._owner.name === 'string') { + ownerName = element._owner.name; + } // Give the component that originally created this child. - childOwner = ` It was passed a child from ${getComponentNameFromType( - element._owner.type, - )}.`; + childOwner = ` It was passed a child from ${ownerName}.`; } setCurrentlyValidatingElement(element); diff --git a/packages/shared/ReactComponentStackFrame.js b/packages/shared/ReactComponentStackFrame.js index c928f191d7b7..b103b760e995 100644 --- a/packages/shared/ReactComponentStackFrame.js +++ b/packages/shared/ReactComponentStackFrame.js @@ -26,10 +26,7 @@ import ReactSharedInternals from 'shared/ReactSharedInternals'; const {ReactCurrentDispatcher} = ReactSharedInternals; let prefix; -export function describeBuiltInComponentFrame( - name: string, - ownerFn: void | null | Function, -): string { +export function describeBuiltInComponentFrame(name: string): string { if (enableComponentStackLocations) { if (prefix === undefined) { // Extract the VM specific prefix used by each line. @@ -43,19 +40,12 @@ export function describeBuiltInComponentFrame( // We use the prefix to ensure our stacks line up with native stack frames. return '\n' + prefix + name; } else { - let ownerName = null; - if (__DEV__ && ownerFn) { - ownerName = ownerFn.displayName || ownerFn.name || null; - } - return describeComponentFrame(name, ownerName); + return describeComponentFrame(name); } } export function describeDebugInfoFrame(name: string, env: ?string): string { - return describeBuiltInComponentFrame( - name + (env ? ' (' + env + ')' : ''), - null, - ); + return describeBuiltInComponentFrame(name + (env ? ' (' + env + ')' : '')); } let reentry = false; @@ -298,29 +288,19 @@ export function describeNativeComponentFrame( return syntheticFrame; } -function describeComponentFrame(name: null | string, ownerName: null | string) { - let sourceInfo = ''; - if (ownerName) { - sourceInfo = ' (created by ' + ownerName + ')'; - } - return '\n in ' + (name || 'Unknown') + sourceInfo; +function describeComponentFrame(name: null | string) { + return '\n in ' + (name || 'Unknown'); } -export function describeClassComponentFrame( - ctor: Function, - ownerFn: void | null | Function, -): string { +export function describeClassComponentFrame(ctor: Function): string { if (enableComponentStackLocations) { return describeNativeComponentFrame(ctor, true); } else { - return describeFunctionComponentFrame(ctor, ownerFn); + return describeFunctionComponentFrame(ctor); } } -export function describeFunctionComponentFrame( - fn: Function, - ownerFn: void | null | Function, -): string { +export function describeFunctionComponentFrame(fn: Function): string { if (enableComponentStackLocations) { return describeNativeComponentFrame(fn, false); } else { @@ -328,11 +308,7 @@ export function describeFunctionComponentFrame( return ''; } const name = fn.displayName || fn.name || null; - let ownerName = null; - if (__DEV__ && ownerFn) { - ownerName = ownerFn.displayName || ownerFn.name || null; - } - return describeComponentFrame(name, ownerName); + return describeComponentFrame(name); } } @@ -341,10 +317,7 @@ function shouldConstruct(Component: Function) { return !!(prototype && prototype.isReactComponent); } -export function describeUnknownElementTypeFrameInDEV( - type: any, - ownerFn: void | null | Function, -): string { +export function describeUnknownElementTypeFrameInDEV(type: any): string { if (!__DEV__) { return ''; } @@ -355,32 +328,32 @@ export function describeUnknownElementTypeFrameInDEV( if (enableComponentStackLocations) { return describeNativeComponentFrame(type, shouldConstruct(type)); } else { - return describeFunctionComponentFrame(type, ownerFn); + return describeFunctionComponentFrame(type); } } if (typeof type === 'string') { - return describeBuiltInComponentFrame(type, ownerFn); + return describeBuiltInComponentFrame(type); } switch (type) { case REACT_SUSPENSE_TYPE: - return describeBuiltInComponentFrame('Suspense', ownerFn); + return describeBuiltInComponentFrame('Suspense'); case REACT_SUSPENSE_LIST_TYPE: - return describeBuiltInComponentFrame('SuspenseList', ownerFn); + return describeBuiltInComponentFrame('SuspenseList'); } if (typeof type === 'object') { switch (type.$$typeof) { case REACT_FORWARD_REF_TYPE: - return describeFunctionComponentFrame(type.render, ownerFn); + return describeFunctionComponentFrame(type.render); case REACT_MEMO_TYPE: // Memo may contain any component type so we recursively resolve it. - return describeUnknownElementTypeFrameInDEV(type.type, ownerFn); + return describeUnknownElementTypeFrameInDEV(type.type); case REACT_LAZY_TYPE: { const lazyComponent: LazyComponent = (type: any); const payload = lazyComponent._payload; const init = lazyComponent._init; try { // Lazy may contain any component type so we recursively resolve it. - return describeUnknownElementTypeFrameInDEV(init(payload), ownerFn); + return describeUnknownElementTypeFrameInDEV(init(payload)); } catch (x) {} } } diff --git a/packages/shared/ReactTypes.js b/packages/shared/ReactTypes.js index 59f3362e0cf8..1c2ea3605467 100644 --- a/packages/shared/ReactTypes.js +++ b/packages/shared/ReactTypes.js @@ -181,6 +181,7 @@ export type Awaited = T extends null | void export type ReactComponentInfo = { +name?: string, +env?: string, + +owner?: null | ReactComponentInfo, }; export type ReactAsyncInfo = { From cbb6f2b5461cdce282c7e47b9c68a0897d393383 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 5 Apr 2024 12:49:25 -0400 Subject: [PATCH 3/4] [Flight] Support Blobs from Server to Client (#28755) We currently support Blobs when passing from Client to Server so this adds it in the other direction for parity - when `enableFlightBinary` is enabled. We intentionally only support the `Blob` type to pass-through, not subtype `File`. That's because passing additional meta data like filename might be an accidental leak. You can still pass a `File` through but it'll appear as a `Blob` on the other side. It's also not possible to create a faithful File subclass in all environments without it actually being backed by a file. This implementation isn't great but at least it works. It creates a few indirections. This is because we need to be able to asynchronously emit the buffers but we have to "block" the parent object from resolving while it's loading. Ideally, we should be able to create the Blob on the client early and then stream in it lazily. Because the Blob API doesn't guarantee that the data is available synchronously. Unfortunately, the native APIs doesn't have this. We could implement custom versions of all the data read APIs but then the blobs still wouldn't work with native APIs. So we just have to wait until Blob accepts a stream in the constructor. We should be able to stream each chunk early in the protocol though even though we can't unblock the parent until they've all loaded. I didn't do this yet mostly because of code structure and I'm lazy. --- .../react-client/src/ReactFlightClient.js | 9 ++++ .../src/__tests__/ReactFlightDOMEdge-test.js | 26 ++++++++++ .../react-server/src/ReactFlightServer.js | 50 +++++++++++++++++++ 3 files changed, 85 insertions(+) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index c0cec0db366c..8a35e97b1679 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -737,6 +737,15 @@ function parseModelString( const data = getOutlinedModel(response, id); return new Set(data); } + case 'B': { + // Blob + if (enableBinaryFlight) { + const id = parseInt(value.slice(2), 16); + const data = getOutlinedModel(response, id); + return new Blob(data.slice(1), {type: data[0]}); + } + return undefined; + } case 'I': { // $Infinity return Infinity; diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js index 8304d9927d37..ada7bb35cae2 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js @@ -5,6 +5,7 @@ * LICENSE file in the root directory of this source tree. * * @emails react-core + * @jest-environment ./scripts/jest/ReactDOMServerIntegrationEnvironment */ 'use strict'; @@ -14,6 +15,9 @@ global.ReadableStream = require('web-streams-polyfill/ponyfill/es6').ReadableStream; global.TextEncoder = require('util').TextEncoder; global.TextDecoder = require('util').TextDecoder; +if (typeof Blob === 'undefined') { + global.Blob = require('buffer').Blob; +} // Don't wait before processing work on the server. // TODO: we can replace this with FlightServer.act(). @@ -326,6 +330,28 @@ describe('ReactFlightDOMEdge', () => { expect(result).toEqual(buffers); }); + // @gate enableBinaryFlight + it('should be able to serialize a blob', async () => { + const bytes = new Uint8Array([ + 123, 4, 10, 5, 100, 255, 244, 45, 56, 67, 43, 124, 67, 89, 100, 20, + ]); + const blob = new Blob([bytes, bytes], { + type: 'application/x-test', + }); + const stream = passThrough( + ReactServerDOMServer.renderToReadableStream(blob), + ); + const result = await ReactServerDOMClient.createFromReadableStream(stream, { + ssrManifest: { + moduleMap: null, + moduleLoading: null, + }, + }); + expect(result instanceof Blob).toBe(true); + expect(result.size).toBe(bytes.length * 2); + expect(await result.arrayBuffer()).toEqual(await blob.arrayBuffer()); + }); + it('warns if passing a this argument to bind() of a server reference', async () => { const ServerModule = serverExports({ greet: function () {}, diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index a8c648881fe2..4c8fa75fec9e 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -239,6 +239,8 @@ export type ReactClientValue = | Array | Map | Set + | $ArrayBufferView + | ArrayBuffer | Date | ReactClientObject | Promise; // Thenable @@ -1229,6 +1231,46 @@ function serializeTypedArray( return serializeByValueID(bufferId); } +function serializeBlob(request: Request, blob: Blob): string { + const id = request.nextChunkId++; + request.pendingChunks++; + + const reader = blob.stream().getReader(); + + const model: Array = [blob.type]; + + function progress( + entry: {done: false, value: Uint8Array} | {done: true, value: void}, + ): Promise | void { + if (entry.done) { + const blobId = outlineModel(request, model); + const blobReference = '$B' + blobId.toString(16); + const processedChunk = encodeReferenceChunk(request, id, blobReference); + request.completedRegularChunks.push(processedChunk); + if (request.destination !== null) { + flushCompletedChunks(request, request.destination); + } + return; + } + // TODO: Emit the chunk early and refer to it later. + model.push(entry.value); + // $FlowFixMe[incompatible-call] + return reader.read().then(progress).catch(error); + } + + function error(reason: mixed) { + const digest = logRecoverableError(request, reason); + emitErrorChunk(request, id, digest, reason); + if (request.destination !== null) { + flushCompletedChunks(request, request.destination); + } + } + // $FlowFixMe[incompatible-call] + reader.read().then(progress).catch(error); + + return '$' + id.toString(16); +} + function escapeStringValue(value: string): string { if (value[0] === '$') { // We need to escape $ prefixed strings since we use those to encode @@ -1606,6 +1648,10 @@ function renderModelDestructive( if (value instanceof DataView) { return serializeTypedArray(request, 'V', value); } + // TODO: Blob is not available in old Node. Remove the typeof check later. + if (typeof Blob === 'function' && value instanceof Blob) { + return serializeBlob(request, value); + } } const iteratorFn = getIteratorFn(value); @@ -2146,6 +2192,10 @@ function renderConsoleValue( if (value instanceof DataView) { return serializeTypedArray(request, 'V', value); } + // TODO: Blob is not available in old Node. Remove the typeof check later. + if (typeof Blob === 'function' && value instanceof Blob) { + return serializeBlob(request, value); + } } const iteratorFn = getIteratorFn(value); From bfd8da807c75a2d123627415f9eaf2d36ac3ed6a Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 5 Apr 2024 13:06:39 -0400 Subject: [PATCH 4/4] Make class prop resolution faster (#28766) `delete` causes an object (in V8, and maybe other engines) to deopt to a dictionary instead of a class. Instead of `assign` + `delete`, manually iterate over all the properties, like the JSX runtime does. To avoid copying the object twice I moved the `ref` prop removal to come before handling default props. If we already cloned the props to remove `ref`, then we can skip cloning again to handle default props. --- .../src/ReactFiberClassComponent.js | 31 +++++++++------ packages/react-server/src/ReactFizzServer.js | 38 ++++++++++++------- 2 files changed, 44 insertions(+), 25 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index e464670bfedb..f802c261ba41 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -1251,7 +1251,19 @@ export function resolveClassComponentProps( ): Object { let newProps = baseProps; - // Resolve default props. Taken from old JSX runtime, where this used to live. + if (enableRefAsProp) { + // Remove ref from the props object, if it exists. + if ('ref' in baseProps) { + newProps = ({}: any); + for (const propName in baseProps) { + if (propName !== 'ref') { + newProps[propName] = baseProps[propName]; + } + } + } + } + + // Resolve default props. const defaultProps = Component.defaultProps; if ( defaultProps && @@ -1259,7 +1271,12 @@ export function resolveClassComponentProps( // default props here in the reconciler, rather than in the JSX runtime. (disableDefaultPropsExceptForClasses || !alreadyResolvedDefaultProps) ) { - newProps = assign({}, newProps, baseProps); + // We may have already copied the props object above to remove ref. If so, + // we can modify that. Otherwise, copy the props object with Object.assign. + if (newProps === baseProps) { + newProps = assign({}, newProps, baseProps); + } + // Taken from old JSX runtime, where this used to live. for (const propName in defaultProps) { if (newProps[propName] === undefined) { newProps[propName] = defaultProps[propName]; @@ -1267,16 +1284,6 @@ export function resolveClassComponentProps( } } - if (enableRefAsProp) { - // Remove ref from the props object, if it exists. - if ('ref' in newProps) { - if (newProps === baseProps) { - newProps = assign({}, newProps); - } - delete newProps.ref; - } - } - return newProps; } diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 327311d74fa3..a322d57df695 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -1397,24 +1397,36 @@ export function resolveClassComponentProps( ): Object { let newProps = baseProps; - // Resolve default props. Taken from old JSX runtime, where this used to live. - const defaultProps = Component.defaultProps; - if (defaultProps && disableDefaultPropsExceptForClasses) { - newProps = assign({}, newProps, baseProps); - for (const propName in defaultProps) { - if (newProps[propName] === undefined) { - newProps[propName] = defaultProps[propName]; + if (enableRefAsProp) { + // Remove ref from the props object, if it exists. + if ('ref' in baseProps) { + newProps = ({}: any); + for (const propName in baseProps) { + if (propName !== 'ref') { + newProps[propName] = baseProps[propName]; + } } } } - if (enableRefAsProp) { - // Remove ref from the props object, if it exists. - if ('ref' in newProps) { - if (newProps === baseProps) { - newProps = assign({}, newProps); + // Resolve default props. + const defaultProps = Component.defaultProps; + if ( + defaultProps && + // If disableDefaultPropsExceptForClasses is true, we always resolve + // default props here, rather than in the JSX runtime. + disableDefaultPropsExceptForClasses + ) { + // We may have already copied the props object above to remove ref. If so, + // we can modify that. Otherwise, copy the props object with Object.assign. + if (newProps === baseProps) { + newProps = assign({}, newProps, baseProps); + } + // Taken from old JSX runtime, where this used to live. + for (const propName in defaultProps) { + if (newProps[propName] === undefined) { + newProps[propName] = defaultProps[propName]; } - delete newProps.ref; } }