Skip to content

Avoid acccessing React internals from use-sync-external-store/shim #29868

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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');
Expand All @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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');
Expand Down
5 changes: 4 additions & 1 deletion packages/use-sync-external-store/src/useSyncExternalStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -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+.' +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 ' +
Expand All @@ -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;
Expand Down
9 changes: 2 additions & 7 deletions scripts/jest/TestFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down