Repository: facebook/react
Pull request: Avoid acccessing React internals from `use-sync-external-store/shim`

Fixes #29854

## Summary
Prior to this change, the build process or `use-sync-external-store` would replace `console.error` with
an `error` call that then again would call `printWarning`, which accesses
`React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED` in current stable
versions (see https://unpkg.com/browse/use-sync-external-store@1.2.2/cjs/use-sync-external-store-shim.development.js)
and `React.__CLIENT_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE` in future versions
(see https://unpkg.com/browse/use-sync-external-store@0.0.0-experimental-a26e3f403e-20240611/cjs/use-sync-external-store-shim.development.js).

This might cause builds to fail, as there would be no version of the package that is compatible with both React 18 and 19.

With this change, that replacement is avoided and a normal `console.error`
call is made instead.

## How did you test this change?

I verified by locally building this locally with `yarn build use-sync-external-store` and looking at the build output at `build/oss-stable-semver/use-sync-external-store/cjs/use-sync-external-store-shim.development.js`.


Changed files:
filename: packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js
patch: @@ -20,7 +20,7 @@ let useState;
 let useEffect;
 let useLayoutEffect;
 let assertLog;
-let originalError;
+let assertConsoleErrorDev;
 
 // This tests shared behavior between the built-in and shim implementations of
 // of useSyncExternalStore.
@@ -50,9 +50,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {
             : 'react-dom-17/umd/react-dom.production.min.js',
         ),
       );
-      // Because React 17 prints extra logs we need to ignore them.
-      originalError = console.error;
-      console.error = jest.fn();
     }
     React = require('react');
     ReactDOM = require('react-dom');
@@ -63,6 +60,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {
     useLayoutEffect = React.useLayoutEffect;
     const InternalTestUtils = require('internal-test-utils');
     assertLog = InternalTestUtils.assertLog;
+    assertConsoleErrorDev = InternalTestUtils.assertConsoleErrorDev;
     const internalAct = require('internal-test-utils').act;
 
     // The internal act implementation doesn't batch updates by default, since
@@ -85,11 +83,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {
     useSyncExternalStoreWithSelector =
       require('use-sync-external-store/shim/with-selector').useSyncExternalStoreWithSelector;
   });
-  afterEach(() => {
-    if (gate(flags => flags.enableUseSyncExternalStoreShim)) {
-      console.error = originalError;
-    }
-  });
   function Text({text}) {
     Scheduler.log(text);
     return text;
@@ -630,36 +623,30 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {
     const container = document.createElement('div');
     const root = createRoot(container);
     await expect(async () => {
-      await expect(async () => {
-        await act(() => {
-          ReactDOM.flushSync(async () =>
-            root.render(React.createElement(App, null)),
-          );
-        });
-      }).rejects.toThrow(
-        'Maximum update depth exceeded. This can happen when a component repeatedly ' +
-          'calls setState inside componentWillUpdate or componentDidUpdate. React limits ' +
-          'the number of nested updates to prevent infinite loops.',
-      );
-    }).toErrorDev(
+      await act(() => {
+        ReactDOM.flushSync(async () =>
+          root.render(React.createElement(App, null)),
+        );
+      });
+    }).rejects.toThrow(
+      'Maximum update depth exceeded. This can happen when a component repeatedly ' +
+        'calls setState inside componentWillUpdate or componentDidUpdate. React limits ' +
+        'the number of nested updates to prevent infinite loops.',
+    );
+
+    assertConsoleErrorDev(
       gate(flags => flags.enableUseSyncExternalStoreShim)
         ? [
-            'Maximum update depth exceeded. ',
-            'The result of getSnapshot should be cached to avoid an infinite loop',
-            'The above error occurred in the',
+            [
+              'The result of getSnapshot should be cached to avoid an infinite loop',
+              {withoutStack: true},
+            ],
+            'Error: Maximum update depth exceeded',
+            'The above error occurred i',
           ]
         : [
             'The result of getSnapshot should be cached to avoid an infinite loop',
           ],
-      {
-        withoutStack: gate(flags => {
-          if (flags.enableUseSyncExternalStoreShim) {
-            // Stacks don't work when mixing the source and the npm package.
-            return flags.source ? 1 : 0;
-          }
-          return false;
-        }),
-      },
     );
   });
   it('getSnapshot can return NaN without infinite loop warning', async () => {
@@ -850,10 +837,9 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {
         // client. To avoid this server mismatch warning, user must account for
         // this themselves and return the correct value inside `getSnapshot`.
         await act(() => {
-          expect(() =>
-            ReactDOM.hydrate(React.createElement(App, null), container),
-          ).toErrorDev('Text content did not match');
+          ReactDOM.hydrate(React.createElement(App, null), container);
         });
+        assertConsoleErrorDev(['Text content did not match']);
         assertLog(['client', 'Passive effect: client']);
       }
       expect(container.textContent).toEqual('client');

filename: packages/use-sync-external-store/src/useSyncExternalStore.js
patch: @@ -16,7 +16,10 @@ import * as React from 'react';
 export const useSyncExternalStore = React.useSyncExternalStore;
 
 if (__DEV__) {
-  console.error(
+  // Avoid transforming the `console.error` call as it would cause the built artifact
+  // to access React internals, which exist under different paths depending on the
+  // React version.
+  console['error'](
     "The main 'use-sync-external-store' entry point is not supported; all it " +
       "does is re-export useSyncExternalStore from the 'react' package, so " +
       'it only works with React 18+.' +

filename: packages/use-sync-external-store/src/useSyncExternalStoreShimClient.js
patch: @@ -40,7 +40,10 @@ export function useSyncExternalStore<T>(
     if (!didWarnOld18Alpha) {
       if (React.startTransition !== undefined) {
         didWarnOld18Alpha = true;
-        console.error(
+        // Avoid transforming the `console.error` call as it would cause the built artifact
+        // to access React internals, which exist under different paths depending on the
+        // React version.
+        console['error'](
           'You are using an outdated, pre-release alpha of React 18 that ' +
             'does not support useSyncExternalStore. The ' +
             'use-sync-external-store shim will not work correctly. Upgrade ' +
@@ -59,7 +62,10 @@ export function useSyncExternalStore<T>(
     if (!didWarnUncachedGetSnapshot) {
       const cachedValue = getSnapshot();
       if (!is(value, cachedValue)) {
-        console.error(
+        // Avoid transforming the `console.error` call as it would cause the built artifact
+        // to access React internals, which exist under different paths depending on the
+        // React version.
+        console['error'](
           'The result of getSnapshot should be cached to avoid an infinite loop',
         );
         didWarnUncachedGetSnapshot = true;

filename: scripts/jest/TestFlags.js
patch: @@ -95,13 +95,8 @@ function getTestFlags() {
 
       // This is used by useSyncExternalStoresShared-test.js to decide whether
       // to test the shim or the native implementation of useSES.
-      // TODO: It's disabled when enableRefAsProp is on because the JSX
-      // runtime used by our tests is not compatible with older versions of
-      // React. If we want to keep testing this shim after enableRefIsProp is
-      // on everywhere, we'll need to find some other workaround. Maybe by
-      // only using createElement instead of JSX in that test module.
-      enableUseSyncExternalStoreShim:
-        !__VARIANT__ && !featureFlags.enableRefAsProp,
+
+      enableUseSyncExternalStoreShim: !__VARIANT__,
 
       // If there's a naming conflict between scheduler and React feature flags, the
       // React ones take precedence.