From f42f8c0635775c3cb8beb5252aa777ff7d468ae5 Mon Sep 17 00:00:00 2001 From: Ricky Date: Fri, 3 Jan 2025 12:53:19 -0500 Subject: [PATCH 1/2] [flags] Remove enableServerComponentLogs (#31772) This has landed everywhere. --- .../react-client/src/ReactFlightClient.js | 44 ++++-------- .../src/__tests__/ReactFlight-test.js | 67 +++++++------------ .../react-server/src/ReactFlightServer.js | 8 +-- packages/shared/ReactFeatureFlags.js | 2 - .../forks/ReactFeatureFlags.native-fb.js | 1 - .../forks/ReactFeatureFlags.native-oss.js | 1 - .../forks/ReactFeatureFlags.test-renderer.js | 1 - ...actFeatureFlags.test-renderer.native-fb.js | 1 - .../ReactFeatureFlags.test-renderer.www.js | 1 - .../shared/forks/ReactFeatureFlags.www.js | 2 - 10 files changed, 37 insertions(+), 91 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 7c424d45b3183..288d6b7c1d018 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -45,7 +45,6 @@ import type {TemporaryReferenceSet} from './ReactFlightTemporaryReferences'; import { enablePostpone, enableOwnerStacks, - enableServerComponentLogs, enableProfilerTimer, enableComponentPerformanceTrack, } from 'shared/ReactFeatureFlags'; @@ -2139,34 +2138,22 @@ function resolveErrorDev( } let error; - if (!enableOwnerStacks && !enableServerComponentLogs) { - // Executing Error within a native stack isn't really limited to owner stacks - // but we gate it behind the same flag for now while iterating. - // eslint-disable-next-line react-internal/prod-error-codes - error = Error( + const callStack = buildFakeCallStack( + response, + stack, + env, + // $FlowFixMe[incompatible-use] + Error.bind( + null, message || 'An error occurred in the Server Components render but no message was provided', - ); - // For backwards compat we use the V8 formatting when the flag is off. - error.stack = formatV8Stack(error.name, error.message, stack); + ), + ); + const rootTask = getRootTask(response, env); + if (rootTask != null) { + error = rootTask.run(callStack); } else { - const callStack = buildFakeCallStack( - response, - stack, - env, - // $FlowFixMe[incompatible-use] - Error.bind( - null, - message || - 'An error occurred in the Server Components render but no message was provided', - ), - ); - const rootTask = getRootTask(response, env); - if (rootTask != null) { - error = rootTask.run(callStack); - } else { - error = callStack(); - } + error = callStack(); } (error: any).environmentName = env; @@ -2699,11 +2686,6 @@ function resolveConsoleEntry( const env = payload[3]; const args = payload.slice(4); - if (!enableOwnerStacks && !enableServerComponentLogs) { - bindToConsole(methodName, args, env)(); - return; - } - replayConsoleWithCallStackInDEV( response, methodName, diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index 980dbbf0e1247..e11c11261406f 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -1377,26 +1377,14 @@ describe('ReactFlight', () => { errors: [ { message: 'This is an error', - stack: gate( - flags => - flags.enableOwnerStacks || flags.enableServerComponentLogs, - ) - ? expect.stringContaining( - 'Error: This is an error\n' + - ' at eval (eval at testFunction (inspected-page.html:29:11),%20%3Canonymous%3E:1:35)\n' + - ' at ServerComponentError (file://~/(some)(really)(exotic-directory)/ReactFlight-test.js:1166:19)\n' + - ' at (file:///testing.js:42:3)\n' + - ' at (file:///testing.js:42:3)\n' + - ' at div (', - ) - : expect.stringContaining( - 'Error: This is an error\n' + - ' at eval (eval at testFunction (inspected-page.html:29:11),%20%3Canonymous%3E:1:10)\n' + - ' at ServerComponentError (file://~/(some)(really)(exotic-directory)/ReactFlight-test.js:1166:19)\n' + - ' at file:///testing.js:42:3\n' + - ' at file:///testing.js:42:3\n' + - ' at div (', - ), + 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' + + ' at ServerComponentError (file://~/(some)(really)(exotic-directory)/ReactFlight-test.js:1166:19)\n' + + ' at (file:///testing.js:42:3)\n' + + ' at (file:///testing.js:42:3)\n' + + ' at div (', + ), digest: 'a dev digest', environmentName: 'Server', }, @@ -1415,18 +1403,16 @@ describe('ReactFlight', () => { ['', 'Server'], [__filename, 'Server'], ] - : gate(flags => flags.enableServerComponentLogs) - ? [ - // TODO: What should we request here? The outer () or the inner (inspected-page.html)? - ['inspected-page.html:29:11), ', 'Server'], - [ - 'file://~/(some)(really)(exotic-directory)/ReactFlight-test.js', - 'Server', - ], - ['file:///testing.js', 'Server'], - ['', 'Server'], - ] - : [], + : [ + // TODO: What should we request here? The outer () or the inner (inspected-page.html)? + ['inspected-page.html:29:11), ', 'Server'], + [ + 'file://~/(some)(really)(exotic-directory)/ReactFlight-test.js', + 'Server', + ], + ['file:///testing.js', 'Server'], + ['', 'Server'], + ], }); } else { expect(errors.map(getErrorForJestMatcher)).toEqual([ @@ -3312,14 +3298,7 @@ describe('ReactFlight', () => { .split('\n') .slice(0, 4) .join('\n') - .replaceAll( - ' (/', - gate( - flags => flags.enableOwnerStacks || flags.enableServerComponentLogs, - ) - ? ' (file:///' - : ' (/', - ); // The eval will end up normalizing these + .replaceAll(' (/', ' (file:///'); // The eval will end up normalizing these let sawReactPrefix = false; const environments = []; @@ -3352,7 +3331,7 @@ describe('ReactFlight', () => { 'third-party', 'third-party', ]); - } else if (__DEV__ && gate(flags => flags.enableServerComponentLogs)) { + } else if (__DEV__) { expect(environments.slice(0, 3)).toEqual([ 'third-party', 'third-party', @@ -3412,7 +3391,7 @@ describe('ReactFlight', () => { expect(ReactNoop).toMatchRenderedOutput(
hi
); }); - // @gate enableServerComponentLogs && __DEV__ && enableOwnerStacks + // @gate __DEV__ && enableOwnerStacks it('replays logs, but not onError logs', async () => { function foo() { return 'hello'; @@ -3493,7 +3472,7 @@ describe('ReactFlight', () => { expect(ownerStacks).toEqual(['\n in App (at **)']); }); - // @gate enableServerComponentLogs && __DEV__ + // @gate __DEV__ it('replays logs with cyclic objects', async () => { const cyclic = {cycle: null}; cyclic.cycle = cyclic; @@ -3764,7 +3743,7 @@ describe('ReactFlight', () => { ); }); - // @gate (enableOwnerStacks && enableServerComponentLogs) || !__DEV__ + // @gate (enableOwnerStacks) || !__DEV__ it('should include only one component stack in replayed logs (if DevTools or polyfill adds them)', () => { class MyError extends Error { toJSON() { diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 5a27f006179ba..67ae695ca0230 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -17,7 +17,6 @@ import { enablePostpone, enableHalt, enableTaint, - enableServerComponentLogs, enableOwnerStacks, enableProfilerTimer, enableComponentPerformanceTrack, @@ -234,12 +233,7 @@ function patchConsole(consoleInst: typeof console, methodName: string) { } } -if ( - enableServerComponentLogs && - __DEV__ && - typeof console === 'object' && - console !== null -) { +if (__DEV__ && typeof console === 'object' && console !== null) { // Instrument console to capture logs for replaying on the client. patchConsole(console, 'assert'); patchConsole(console, 'debug'); diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index f1332a2bfd1ec..a8ab44136190e 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -128,8 +128,6 @@ export const alwaysThrottleRetries = true; export const passChildrenWhenCloningPersistedNodes = false; -export const enableServerComponentLogs = true; - /** * Enables a new Fiber flag used in persisted mode to reduce the number * of cloned host components. diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index ed06661eedc39..7fd4fd7dfe69d 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -66,7 +66,6 @@ export const enableRetryLaneExpiration = false; export const enableSchedulingProfiler = __PROFILE__; export const enableComponentPerformanceTrack = false; export const enableScopeAPI = false; -export const enableServerComponentLogs = true; export const enableSuspenseAvoidThisFallback = false; export const enableSuspenseCallback = true; export const enableTaint = true; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 2220891b7b426..8a74a9e9dde9d 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -52,7 +52,6 @@ export const enableRetryLaneExpiration = false; export const enableSchedulingProfiler = __PROFILE__; export const enableComponentPerformanceTrack = false; export const enableScopeAPI = false; -export const enableServerComponentLogs = true; export const enableShallowPropDiffing = false; export const enableSuspenseAvoidThisFallback = false; export const enableSuspenseCallback = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 2c5bc8f297ba5..7156814474571 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -61,7 +61,6 @@ export const passChildrenWhenCloningPersistedNodes = false; export const enablePersistedModeClonedFlag = false; export const disableClientCache = true; -export const enableServerComponentLogs = true; export const enableInfiniteRenderLoopDetection = false; export const renameElementSymbol = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js index 1c2fbb4847960..1777671e4eef1 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js @@ -49,7 +49,6 @@ export const enableRetryLaneExpiration = false; export const enableSchedulingProfiler = __PROFILE__; export const enableComponentPerformanceTrack = false; export const enableScopeAPI = false; -export const enableServerComponentLogs = true; export const enableShallowPropDiffing = false; export const enableSuspenseAvoidThisFallback = false; export const enableSuspenseCallback = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 8e96038f44246..ab2dd11939382 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -63,7 +63,6 @@ export const passChildrenWhenCloningPersistedNodes = false; export const enablePersistedModeClonedFlag = false; export const disableClientCache = true; -export const enableServerComponentLogs = true; export const enableInfiniteRenderLoopDetection = false; export const enableReactTestRendererWarning = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 658443aea13e9..6403d3592ba74 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -104,8 +104,6 @@ export const enablePersistedModeClonedFlag = false; export const enableAsyncDebugInfo = false; export const disableClientCache = true; -export const enableServerComponentLogs = true; - export const enableReactTestRendererWarning = false; export const disableLegacyMode = true; From bf883bebbc4973dea0e4801a5a62f82043ff57ee Mon Sep 17 00:00:00 2001 From: Ricky Date: Fri, 3 Jan 2025 12:53:28 -0500 Subject: [PATCH 2/2] [fizz] fix empty string href double warning (#31783) I think this is the suggested change from https://github.com/facebook/react/pull/31765#discussion_r1884541447 But no tests fail and I'm not sure how to test it? Seems sus. Also seems like the `removeAttribute` here should be changed? https://github.com/facebook/react/blob/9d9f12f2699a049777fa88914306ad4de9e2b74d/packages/react-dom-bindings/src/client/ReactDOMComponent.js#L400-L427 --- .../src/client/ReactDOMComponent.js | 27 +++++++------------ .../src/client/ReactFiberConfigDOM.js | 6 +++-- ...eactDOMServerIntegrationAttributes-test.js | 23 +++++++++------- 3 files changed, 27 insertions(+), 29 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js index 05892c930e1ca..2266021b9f60c 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMComponent.js +++ b/packages/react-dom-bindings/src/client/ReactDOMComponent.js @@ -2510,26 +2510,17 @@ function diffHydratedGenericElement( ); } } - hydrateSanitizedAttribute( - domElement, - propKey, - propKey, - null, - extraAttributes, - serverDifferences, - ); - continue; - } else { - hydrateSanitizedAttribute( - domElement, - propKey, - propKey, - value, - extraAttributes, - serverDifferences, - ); continue; } + hydrateSanitizedAttribute( + domElement, + propKey, + propKey, + value, + extraAttributes, + serverDifferences, + ); + continue; case 'action': case 'formAction': { const serverValue = domElement.getAttribute(propKey); diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index 8e94d48beeef2..ea79903de02d6 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -1139,7 +1139,9 @@ export function canHydrateInstance( } else if ( rel !== anyProps.rel || element.getAttribute('href') !== - (anyProps.href == null ? null : anyProps.href) || + (anyProps.href == null || anyProps.href === '' + ? null + : anyProps.href) || element.getAttribute('crossorigin') !== (anyProps.crossOrigin == null ? null : anyProps.crossOrigin) || element.getAttribute('title') !== @@ -2984,7 +2986,7 @@ export function hydrateHoistable( const node = nodes[i]; if ( node.getAttribute('href') !== - (props.href == null ? null : props.href) || + (props.href == null || props.href === '' ? null : props.href) || node.getAttribute('rel') !== (props.rel == null ? null : props.rel) || node.getAttribute('title') !== diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js index f683a14d9aeff..7a52e9d0dd0a8 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js @@ -62,18 +62,23 @@ describe('ReactDOMServerIntegration', () => { expect(e.getAttribute('href')).toBe(''); }); - itRenders('empty href on other tags', async render => { + itRenders('empty href on base tags as null', async render => { + const e = await render(, 1); + expect(e.getAttribute('href')).toBe(null); + }); + + itRenders('empty href on area tags as null', async render => { const e = await render( - // would be more sensible. - // However, that results in a hydration warning as well. - // Our test helpers do not support different error counts for initial - // server render and hydration. - // The number of errors on the server need to be equal to the number of - // errors during hydration. - // So we use a
instead. -
, + + + , 1, ); + expect(e.firstChild.getAttribute('href')).toBe(null); + }); + + itRenders('empty href on link tags as null', async render => { + const e = await render(, 1); expect(e.getAttribute('href')).toBe(null); });