From d8554637372934dd05343312c178e4e2a8f3934e Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 16 Jan 2024 12:50:22 -0500 Subject: [PATCH] Rewrite element tests to not use cache() Because we haven't decided on the client caching semantics of cache(), we're going to remove the behavior for now and add it back later. So we need to rewrite these client tests to not depend on cache(). This essentially reverts the test suite back to what it was before #25506. There were also some tests in the ReactUse-test module that depended on cache() too, which I've also updated. --- .../src/__tests__/ReactCacheElement-test.js | 174 ++++++++---------- .../src/__tests__/ReactUse-test.js | 51 ++--- 2 files changed, 110 insertions(+), 115 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactCacheElement-test.js b/packages/react-reconciler/src/__tests__/ReactCacheElement-test.js index ea84a0ae84ef5..547aedb4cc10f 100644 --- a/packages/react-reconciler/src/__tests__/ReactCacheElement-test.js +++ b/packages/react-reconciler/src/__tests__/ReactCacheElement-test.js @@ -2,6 +2,7 @@ let React; let ReactNoop; let Cache; let getCacheSignal; +let getCacheForType; let Scheduler; let assertLog; let act; @@ -10,13 +11,11 @@ let Activity; let useCacheRefresh; let startTransition; let useState; -let cache; -let getTextCache; let textCaches; let seededCache; -describe('ReactCache', () => { +describe('ReactCacheElement', () => { beforeEach(() => { jest.resetModules(); @@ -26,9 +25,9 @@ describe('ReactCache', () => { Scheduler = require('scheduler'); act = require('internal-test-utils').act; Suspense = React.Suspense; - cache = React.cache; Activity = React.unstable_Activity; getCacheSignal = React.unstable_getCacheSignal; + getCacheForType = React.unstable_getCacheForType; useCacheRefresh = React.unstable_useCacheRefresh; startTransition = React.startTransition; useState = React.useState; @@ -38,59 +37,57 @@ describe('ReactCache', () => { textCaches = []; seededCache = null; + }); - if (gate(flags => flags.enableCache)) { - getTextCache = cache(() => { - if (seededCache !== null) { - // Trick to seed a cache before it exists. - // TODO: Need a built-in API to seed data before the initial render (i.e. - // not a refresh because nothing has mounted yet). - const textCache = seededCache; - seededCache = null; - return textCache; - } - - const data = new Map(); - const version = textCaches.length + 1; - const textCache = { - version, - data, - resolve(text) { - const record = data.get(text); - if (record === undefined) { - const newRecord = { - status: 'resolved', - value: text, - cleanupScheduled: false, - }; - data.set(text, newRecord); - } else if (record.status === 'pending') { - record.value.resolve(); - } - }, - reject(text, error) { - const record = data.get(text); - if (record === undefined) { - const newRecord = { - status: 'rejected', - value: error, - cleanupScheduled: false, - }; - data.set(text, newRecord); - } else if (record.status === 'pending') { - record.value.reject(); - } - }, - }; - textCaches.push(textCache); - return textCache; - }); + function createTextCache() { + if (seededCache !== null) { + // Trick to seed a cache before it exists. + // TODO: Need a built-in API to seed data before the initial render (i.e. + // not a refresh because nothing has mounted yet). + const textCache = seededCache; + seededCache = null; + return textCache; } - }); + + const data = new Map(); + const version = textCaches.length + 1; + const textCache = { + version, + data, + resolve(text) { + const record = data.get(text); + if (record === undefined) { + const newRecord = { + status: 'resolved', + value: text, + cleanupScheduled: false, + }; + data.set(text, newRecord); + } else if (record.status === 'pending') { + record.value.resolve(); + } + }, + reject(text, error) { + const record = data.get(text); + if (record === undefined) { + const newRecord = { + status: 'rejected', + value: error, + cleanupScheduled: false, + }; + data.set(text, newRecord); + } else if (record.status === 'pending') { + record.value.reject(); + } + }, + }; + textCaches.push(textCache); + return textCache; + } function readText(text) { const signal = getCacheSignal ? getCacheSignal() : null; - const textCache = getTextCache(); + const textCache = getCacheForType(createTextCache); const record = textCache.data.get(text); if (record !== undefined) { if (!record.cleanupScheduled) { @@ -167,7 +164,7 @@ describe('ReactCache', () => { function seedNextTextCache(text) { if (seededCache === null) { - seededCache = getTextCache(); + seededCache = createTextCache(); } seededCache.resolve(text); } @@ -182,7 +179,7 @@ describe('ReactCache', () => { } } - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('render Cache component', async () => { const root = ReactNoop.createRoot(); await act(() => { @@ -191,7 +188,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Hi'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('mount new data', async () => { const root = ReactNoop.createRoot(); await act(() => { @@ -247,7 +244,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('multiple new Cache boundaries in the same mount share the same, fresh root cache', async () => { function App() { return ( @@ -290,7 +287,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('multiple new Cache boundaries in the same update share the same, fresh cache', async () => { function App({showMore}) { return showMore ? ( @@ -342,7 +339,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test( 'nested cache boundaries share the same cache as the root during ' + 'the initial render', @@ -382,7 +379,7 @@ describe('ReactCache', () => { }, ); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('new content inside an existing Cache boundary should re-use already cached data', async () => { function App({showMore}) { return ( @@ -426,7 +423,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('a new Cache boundary uses fresh cache', async () => { // The only difference from the previous test is that the "Show More" // content is wrapped in a nested boundary @@ -484,7 +481,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye!'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('inner/outer cache boundaries uses the same cache instance on initial render', async () => { const root = ReactNoop.createRoot(); @@ -566,7 +563,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('inner/ outer cache boundaries added in the same update use the same cache instance', async () => { const root = ReactNoop.createRoot(); @@ -655,7 +652,6 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye'); }); - // @gate enableCache test('refresh a cache boundary', async () => { let refresh; function App() { @@ -705,7 +701,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('refresh the root cache', async () => { let refresh; function App() { @@ -753,7 +749,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('refresh the root cache without a transition', async () => { let refresh; function App() { @@ -808,20 +804,11 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('refresh a cache with seed data', async () => { - let refreshWithSeed; + let refresh; function App() { - const refresh = useCacheRefresh(); - const [seed, setSeed] = useState({fn: null}); - if (seed.fn) { - seed.fn(); - seed.fn = null; - } - refreshWithSeed = fn => { - setSeed({fn}); - refresh(); - }; + refresh = useCacheRefresh(); return ; } @@ -851,12 +838,11 @@ describe('ReactCache', () => { // server mutation. // TODO: Seeding multiple typed textCaches. Should work by calling `refresh` // multiple times with different key/value pairs - startTransition(() => - refreshWithSeed(() => { - const textCache = getTextCache(); - textCache.resolve('A'); - }), - ); + startTransition(() => { + const textCache = createTextCache(); + textCache.resolve('A'); + startTransition(() => refresh(createTextCache, textCache)); + }); }); // The root should re-render without a cache miss. // The cache is not cleared up yet, since it's still reference by the root @@ -871,7 +857,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('refreshing a parent cache also refreshes its children', async () => { let refreshShell; function RefreshShell() { @@ -946,7 +932,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye!'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test( 'refreshing a cache boundary does not refresh the other boundaries ' + 'that mounted at the same time (i.e. the ones that share the same cache)', @@ -1027,7 +1013,7 @@ describe('ReactCache', () => { }, ); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test( 'mount a new Cache boundary in a sibling while simultaneously ' + 'resolving a Suspense boundary', @@ -1096,7 +1082,7 @@ describe('ReactCache', () => { }, ); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('cache pool is cleared once transitions that depend on it commit their shell', async () => { function Child({text}) { return ( @@ -1189,7 +1175,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye!'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('cache pool is not cleared by arbitrary commits', async () => { function App() { return ( @@ -1268,7 +1254,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye!'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('cache boundary uses a fresh cache when its key changes', async () => { const root = ReactNoop.createRoot(); seedNextTextCache('A'); @@ -1307,7 +1293,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye!'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('overlapping transitions after an initial mount use the same fresh cache', async () => { const root = ReactNoop.createRoot(); await act(() => { @@ -1375,7 +1361,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye!'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('overlapping updates after an initial mount use the same fresh cache', async () => { const root = ReactNoop.createRoot(); await act(() => { @@ -1438,7 +1424,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye!'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test('cleans up cache only used in an aborted transition', async () => { const root = ReactNoop.createRoot(); seedNextTextCache('A'); @@ -1493,7 +1479,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye!'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test.skip('if a root cache refresh never commits its fresh cache is released', async () => { const root = ReactNoop.createRoot(); let refresh; @@ -1535,7 +1521,7 @@ describe('ReactCache', () => { expect(root).toMatchRenderedOutput('Bye!'); }); - // @gate enableCacheElement && enableCache + // @gate enableCacheElement test.skip('if a cache boundary refresh never commits its fresh cache is released', async () => { const root = ReactNoop.createRoot(); let refresh; diff --git a/packages/react-reconciler/src/__tests__/ReactUse-test.js b/packages/react-reconciler/src/__tests__/ReactUse-test.js index 7f96506260d1b..fd49f8c8d784f 100644 --- a/packages/react-reconciler/src/__tests__/ReactUse-test.js +++ b/packages/react-reconciler/src/__tests__/ReactUse-test.js @@ -11,7 +11,6 @@ let useMemo; let useEffect; let Suspense; let startTransition; -let cache; let pendingTextRequests; let waitFor; let waitForPaint; @@ -34,7 +33,6 @@ describe('ReactUse', () => { useEffect = React.useEffect; Suspense = React.Suspense; startTransition = React.startTransition; - cache = React.cache; const InternalTestUtils = require('internal-test-utils'); waitForAll = InternalTestUtils.waitForAll; @@ -643,10 +641,10 @@ describe('ReactUse', () => { }); test('when waiting for data to resolve, an update on a different root does not cause work to be dropped', async () => { - const getCachedAsyncText = cache(getAsyncText); + const promise = getAsyncText('Hi'); function App() { - return ; + return ; } const root1 = ReactNoop.createRoot(); @@ -998,39 +996,46 @@ describe('ReactUse', () => { ); test('load multiple nested Suspense boundaries', async () => { - const getCachedAsyncText = cache(getAsyncText); + const promiseA = getAsyncText('A'); + const promiseB = getAsyncText('B'); + const promiseC = getAsyncText('C'); + assertLog([ + 'Async text requested [A]', + 'Async text requested [B]', + 'Async text requested [C]', + ]); - function AsyncText({text}) { - return ; + function AsyncText({promise}) { + return ; } const root = ReactNoop.createRoot(); await act(() => { root.render( }> - + }> - + }> - + , ); }); - assertLog(['Async text requested [A]', '(Loading A...)']); + assertLog(['(Loading A...)']); expect(root).toMatchRenderedOutput('(Loading A...)'); await act(() => { resolveTextRequests('A'); }); - assertLog(['A', 'Async text requested [B]', '(Loading B...)']); + assertLog(['A', '(Loading B...)']); expect(root).toMatchRenderedOutput('A(Loading B...)'); await act(() => { resolveTextRequests('B'); }); - assertLog(['B', 'Async text requested [C]', '(Loading C...)']); + assertLog(['B', '(Loading C...)']); expect(root).toMatchRenderedOutput('AB(Loading C...)'); await act(() => { @@ -1584,34 +1589,38 @@ describe('ReactUse', () => { }); test('regression test: updates while component is suspended should not be mistaken for render phase updates', async () => { - const getCachedAsyncText = cache(getAsyncText); + const promiseA = getAsyncText('A'); + const promiseB = getAsyncText('B'); + const promiseC = getAsyncText('C'); + assertLog([ + 'Async text requested [A]', + 'Async text requested [B]', + 'Async text requested [C]', + ]); let setState; function App() { - const [state, _setState] = useState('A'); + const [state, _setState] = useState(promiseA); setState = _setState; - return ; + return ; } // Initial render const root = ReactNoop.createRoot(); await act(() => root.render()); - assertLog(['Async text requested [A]']); expect(root).toMatchRenderedOutput(null); await act(() => resolveTextRequests('A')); assertLog(['A']); expect(root).toMatchRenderedOutput('A'); // Update to B. This will suspend. - await act(() => startTransition(() => setState('B'))); - assertLog(['Async text requested [B]']); + await act(() => startTransition(() => setState(promiseB))); expect(root).toMatchRenderedOutput('A'); // While B is suspended, update to C. This should immediately interrupt // the render for B. In the regression, this update was mistakenly treated // as a render phase update. - ReactNoop.flushSync(() => setState('C')); - assertLog(['Async text requested [C]']); + ReactNoop.flushSync(() => setState(promiseC)); // Finish rendering. await act(() => resolveTextRequests('C'));