From f7fc9f941b92e7f1dfa3899c9aacfa8f5e9f2786 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Wed, 2 Nov 2022 15:59:50 -0700 Subject: [PATCH 1/4] Upgrade React Transition Group --- .../combobox/test/ComboBox.test.js | 97 ++++++++++++++++--- packages/@react-spectrum/list/package.json | 2 +- .../@react-spectrum/overlays/package.json | 2 +- .../overlays/src/OpenTransition.tsx | 2 +- .../virtualizer/src/useVirtualizerState.ts | 43 +++++--- yarn.lock | 30 +++--- 6 files changed, 128 insertions(+), 48 deletions(-) diff --git a/packages/@react-spectrum/combobox/test/ComboBox.test.js b/packages/@react-spectrum/combobox/test/ComboBox.test.js index d29701fcbba..c6bab06565a 100644 --- a/packages/@react-spectrum/combobox/test/ComboBox.test.js +++ b/packages/@react-spectrum/combobox/test/ComboBox.test.js @@ -2024,23 +2024,55 @@ describe('ComboBox', function () { ); let button = getByRole('button'); + let combobox = getByRole('combobox'); - await waitFor(() => expect(load).toHaveBeenCalledTimes(1)); + expect(load).toHaveBeenCalledTimes(1); expect(onOpenChange).toHaveBeenCalledTimes(0); expect(onLoadMore).toHaveBeenCalledTimes(0); + // open menu await act(async () => { triggerPress(button); - jest.runAllTimers(); + // advance to open state from Transition + jest.advanceTimersToNextTimer(); }); - let listbox = getByRole('listbox'); expect(listbox).toBeVisible(); + jest.spyOn(listbox, 'clientHeight', 'get').mockImplementation(() => 100); + // update size, virtualizer raf kicks in + act(() => {jest.advanceTimersToNextTimer();}); + // onLoadMore queued by previous timer, run it now + act(() => {jest.advanceTimersToNextTimer();}); expect(onOpenChange).toHaveBeenCalledTimes(1); expect(onOpenChange).toHaveBeenCalledWith(true, 'manual'); expect(onLoadMore).toHaveBeenCalledTimes(1); - await waitFor(() => expect(load).toHaveBeenCalledTimes(1)); + expect(load).toHaveBeenCalledTimes(1); + + if (React.version.startsWith('17') || React.version.startsWith('16')) { + // close menu + act(() => { + combobox.blur(); + // on exiting + jest.advanceTimersToNextTimer(); + // raf from virtualizer relayout + jest.advanceTimersToNextTimer(); + // advance to onExited + jest.advanceTimersToNextTimer(); + }); + } else { + // close menu + act(() => { + combobox.blur(); + // raf from virtualizer relayout + jest.advanceTimersToNextTimer(); + }); + // previous act wraps up onExiting + act(() => { + // advance to onExited + jest.advanceTimersToNextTimer(); + }); + } }); it('onLoadMore is not called on when previously opened', async () => { @@ -2052,29 +2084,58 @@ describe('ComboBox', function () { let button = getByRole('button'); let combobox = getByRole('combobox'); - await waitFor(() => expect(load).toHaveBeenCalledTimes(1)); + expect(load).toHaveBeenCalledTimes(1); expect(onOpenChange).toHaveBeenCalledTimes(0); expect(onLoadMore).toHaveBeenCalledTimes(0); // this call and the one below are more correct for how the code should - // behave, the inital call would have a height of zero and after that a measureable height + // behave, the initial call would have a height of zero and after that a measureable height clientHeightSpy.mockRestore(); clientHeightSpy = jest.spyOn(window.HTMLElement.prototype, 'clientHeight', 'get').mockImplementationOnce(() => 0).mockImplementation(() => 40); // open menu await act(async () => { triggerPress(button); - jest.runAllTimers(); + // advance to open state from Transition + jest.advanceTimersToNextTimer(); }); + let listbox = getByRole('listbox'); + expect(listbox).toBeVisible(); + jest.spyOn(listbox, 'clientHeight', 'get').mockImplementation(() => 100); + // update size, virtualizer raf kicks in + act(() => {jest.advanceTimersToNextTimer();}); + // onLoadMore queued by previous timer, run it now + act(() => {jest.advanceTimersToNextTimer();}); expect(onOpenChange).toHaveBeenCalledTimes(1); expect(onOpenChange).toHaveBeenCalledWith(true, 'manual'); expect(onLoadMore).toHaveBeenCalledTimes(1); - // close menu - act(() => { - combobox.blur(); - jest.runAllTimers(); - }); + if (React.version.startsWith('17') || React.version.startsWith('16')) { + // close menu + act(() => { + combobox.blur(); + // on exiting + jest.advanceTimersToNextTimer(); + // raf from virtualizer relayout + jest.advanceTimersToNextTimer(); + // advance to onExited + jest.advanceTimersToNextTimer(); + }); + } else { + // close menu + act(() => { + combobox.blur(); + // raf from virtualizer relayout + jest.advanceTimersToNextTimer(); + }); + // previous act wraps up onExiting + act(() => { + // advance to onExited + jest.advanceTimersToNextTimer(); + }); + } + + expect(listbox).not.toBeInTheDocument(); expect(onOpenChange).toHaveBeenCalledTimes(2); expect(onOpenChange).toHaveBeenLastCalledWith(false, undefined); @@ -2085,8 +2146,16 @@ describe('ComboBox', function () { // reopen menu await act(async () => { triggerPress(button); - jest.runAllTimers(); + // advance to open state from Transition + jest.advanceTimersToNextTimer(); }); + listbox = getByRole('listbox'); + expect(listbox).toBeVisible(); + jest.spyOn(listbox, 'clientHeight', 'get').mockImplementation(() => 100); + // update size, virtualizer raf kicks in + act(() => {jest.advanceTimersToNextTimer();}); + // onLoadMore queued by previous timer, run it now + act(() => {jest.advanceTimersToNextTimer();}); expect(onOpenChange).toHaveBeenCalledTimes(3); expect(onOpenChange).toHaveBeenLastCalledWith(true, 'manual'); @@ -2095,7 +2164,7 @@ describe('ComboBox', function () { // because the browser limits the popover height via calculatePosition(), // while the test doesn't, causing virtualizer to try to load more expect(onLoadMore).toHaveBeenCalledTimes(2); - await waitFor(() => expect(load).toHaveBeenCalledTimes(1)); + expect(load).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/@react-spectrum/list/package.json b/packages/@react-spectrum/list/package.json index de372a3f4bb..10abb66746f 100644 --- a/packages/@react-spectrum/list/package.json +++ b/packages/@react-spectrum/list/package.json @@ -52,7 +52,7 @@ "@react-types/grid": "^3.1.4", "@react-types/shared": "^3.15.0", "@spectrum-icons/ui": "^3.3.3", - "react-transition-group": "^2.2.0" + "react-transition-group": "^4.4.5" }, "devDependencies": { "@adobe/spectrum-css-temp": "^3.0.0-alpha.1", diff --git a/packages/@react-spectrum/overlays/package.json b/packages/@react-spectrum/overlays/package.json index 72a37440e7e..4821a6f860c 100644 --- a/packages/@react-spectrum/overlays/package.json +++ b/packages/@react-spectrum/overlays/package.json @@ -38,7 +38,7 @@ "@react-stately/overlays": "^3.4.2", "@react-types/overlays": "^3.6.4", "@react-types/shared": "^3.15.0", - "react-transition-group": "^2.2.0" + "react-transition-group": "^4.4.5" }, "devDependencies": { "@adobe/spectrum-css-temp": "3.0.0-alpha.1" diff --git a/packages/@react-spectrum/overlays/src/OpenTransition.tsx b/packages/@react-spectrum/overlays/src/OpenTransition.tsx index bf41d647a99..a7955ebb335 100644 --- a/packages/@react-spectrum/overlays/src/OpenTransition.tsx +++ b/packages/@react-spectrum/overlays/src/OpenTransition.tsx @@ -11,7 +11,7 @@ */ import React from 'react'; -import Transition from 'react-transition-group/Transition'; +import {Transition} from 'react-transition-group'; const OPEN_STATES = { entering: false, diff --git a/packages/@react-stately/virtualizer/src/useVirtualizerState.ts b/packages/@react-stately/virtualizer/src/useVirtualizerState.ts index 4f6e2a07ccf..ce10965b630 100644 --- a/packages/@react-stately/virtualizer/src/useVirtualizerState.ts +++ b/packages/@react-stately/virtualizer/src/useVirtualizerState.ts @@ -11,7 +11,7 @@ */ import {Collection} from '@react-types/shared'; -import {Key, useCallback, useEffect, useMemo, useState} from 'react'; +import {Key, useCallback, useEffect, useMemo, useRef, useState} from 'react'; import {Layout} from './Layout'; import {Rect} from './Rect'; import {ReusableView} from './ReusableView'; @@ -80,22 +80,37 @@ export function useVirtualizerState(opts: VirtualizerPro // eslint-disable-next-line react-hooks/exhaustive-deps }, []); - return { + let setVisibleRect = useCallback((rect) => { + virtualizer.visibleRect = rect; + }, [virtualizer]); + let startScrolling = useCallback(() => { + virtualizer.startScrolling(); + setScrolling(true); + }, [virtualizer]); + let endScrolling = useCallback(() => { + virtualizer.endScrolling(); + setScrolling(false); + }, [virtualizer]); + + let state = useMemo(() => ({ virtualizer, visibleViews, - setVisibleRect: useCallback((rect) => { - virtualizer.visibleRect = rect; - }, [virtualizer]), + setVisibleRect, contentSize, isAnimating, isScrolling, - startScrolling: useCallback(() => { - virtualizer.startScrolling(); - setScrolling(true); - }, [virtualizer]), - endScrolling: useCallback(() => { - virtualizer.endScrolling(); - setScrolling(false); - }, [virtualizer]) - }; + startScrolling, + endScrolling + }), [ + virtualizer, + visibleViews, + setVisibleRect, + contentSize, + isAnimating, + isScrolling, + startScrolling, + endScrolling + ]); + + return state; } diff --git a/yarn.lock b/yarn.lock index a35be1cf38d..e9ac4f6b70b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1120,7 +1120,7 @@ core-js-pure "^3.0.0" regenerator-runtime "^0.13.4" -"@babel/runtime@7.12.5", "@babel/runtime@^7.0.0", "@babel/runtime@^7.1.2", "@babel/runtime@^7.10.2", "@babel/runtime@^7.12.1", "@babel/runtime@^7.12.5", "@babel/runtime@^7.17.8", "@babel/runtime@^7.18.9", "@babel/runtime@^7.5.0", "@babel/runtime@^7.6.2", "@babel/runtime@^7.7.6", "@babel/runtime@^7.8.4", "@babel/runtime@^7.9.2": +"@babel/runtime@7.12.5", "@babel/runtime@^7.0.0", "@babel/runtime@^7.10.2", "@babel/runtime@^7.12.1", "@babel/runtime@^7.12.5", "@babel/runtime@^7.17.8", "@babel/runtime@^7.18.9", "@babel/runtime@^7.5.0", "@babel/runtime@^7.5.5", "@babel/runtime@^7.6.2", "@babel/runtime@^7.7.6", "@babel/runtime@^7.8.4", "@babel/runtime@^7.8.7", "@babel/runtime@^7.9.2": version "7.12.5" resolved "https://registry.yarnpkg.com/@babel/runtime/-/runtime-7.12.5.tgz#410e7e487441e1b360c29be715d870d9b985882e" integrity sha512-plcc+hbExy3McchJCEQG3knOsuh3HH+Prx1P6cLIkET/0dLuQDEnrT+s27Axgc9bqfsmNUNHfscgMUdBpC9xfg== @@ -8687,12 +8687,13 @@ dom-converter@^0.2: dependencies: utila "~0.4" -dom-helpers@^3.4.0: - version "3.4.0" - resolved "https://registry.yarnpkg.com/dom-helpers/-/dom-helpers-3.4.0.tgz#e9b369700f959f62ecde5a6babde4bccd9169af8" - integrity sha512-LnuPJ+dwqKDIyotW1VzmOZ5TONUN7CwkCR5hrgawTUbkBGYdeoNLZo6nNfGkCrjtE1nXXaj7iMMpDa8/d9WoIA== +dom-helpers@^5.0.1: + version "5.2.1" + resolved "https://registry.yarnpkg.com/dom-helpers/-/dom-helpers-5.2.1.tgz#d9400536b2bf8225ad98fe052e029451ac40e902" + integrity sha512-nRCa7CK3VTrM2NmGkIy4cbK7IZlgBE/PYMn55rrXefr5xXDP0LdtfPnblFDoVdcAfslJ7or6iqAUnx0CCGIWQA== dependencies: - "@babel/runtime" "^7.1.2" + "@babel/runtime" "^7.8.7" + csstype "^3.0.2" dom-serializer@0: version "0.2.2" @@ -18618,11 +18619,6 @@ react-is@^16.12.0, react-is@^16.13.1, react-is@^16.8.6: resolved "https://registry.yarnpkg.com/react-is/-/react-is-16.13.1.tgz#789729a4dc36de2999dc156dd6c1d9c18cea56a4" integrity sha512-24e6ynE2H+OKt4kqsOvNd8kBpV65zoxbA4BVsEOB3ARVWQki/DHzaUoC5KuON/BiccDaCCTZBuOcfZs70kR8bQ== -react-lifecycles-compat@^3.0.4: - version "3.0.4" - resolved "https://registry.yarnpkg.com/react-lifecycles-compat/-/react-lifecycles-compat-3.0.4.tgz#4f1a273afdfc8f3488a8c516bfda78f872352362" - integrity sha512-fBASbA6LnOU9dOU2eW7aQ8xmYBSXUIWr+UmF9b1efZBazGNO+rcXT/icdKnYm2pTwcRylVUYwW7H1PHfLekVzA== - react-lowlight@^2.0.0: version "2.0.0" resolved "https://registry.yarnpkg.com/react-lowlight/-/react-lowlight-2.0.0.tgz#3c9fa072c6516add8552f9470287e24e13d38eff" @@ -18656,15 +18652,15 @@ react-test-renderer@^16.9.0: react-is "^16.8.6" scheduler "^0.16.2" -react-transition-group@^2.2.0: - version "2.9.0" - resolved "https://registry.yarnpkg.com/react-transition-group/-/react-transition-group-2.9.0.tgz#df9cdb025796211151a436c69a8f3b97b5b07c8d" - integrity sha512-+HzNTCHpeQyl4MJ/bdE0u6XRMe9+XG/+aL4mCxVN4DnPBQ0/5bfHWPDuOZUzYdMj94daZaZdCCc1Dzt9R/xSSg== +react-transition-group@^4.4.5: + version "4.4.5" + resolved "https://registry.yarnpkg.com/react-transition-group/-/react-transition-group-4.4.5.tgz#e53d4e3f3344da8521489fbef8f2581d42becdd1" + integrity sha512-pZcd1MCJoiKiBR2NRxeCRg13uCXbydPnmB4EOeRrY7480qNWO8IIgQG6zlDkm6uRMsURXPuKq0GWtiM59a5Q6g== dependencies: - dom-helpers "^3.4.0" + "@babel/runtime" "^7.5.5" + dom-helpers "^5.0.1" loose-envify "^1.4.0" prop-types "^15.6.2" - react-lifecycles-compat "^3.0.4" react@^18.0.0: version "18.1.0" From ac74835bac327314a7e1613eae844ee8c0c0bb19 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Wed, 2 Nov 2022 16:01:30 -0700 Subject: [PATCH 2/4] fix lint --- packages/@react-stately/virtualizer/src/useVirtualizerState.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@react-stately/virtualizer/src/useVirtualizerState.ts b/packages/@react-stately/virtualizer/src/useVirtualizerState.ts index ce10965b630..9332309627b 100644 --- a/packages/@react-stately/virtualizer/src/useVirtualizerState.ts +++ b/packages/@react-stately/virtualizer/src/useVirtualizerState.ts @@ -11,7 +11,7 @@ */ import {Collection} from '@react-types/shared'; -import {Key, useCallback, useEffect, useMemo, useRef, useState} from 'react'; +import {Key, useCallback, useEffect, useMemo, useState} from 'react'; import {Layout} from './Layout'; import {Rect} from './Rect'; import {ReusableView} from './ReusableView'; From 965ee6c98e2822287f683f2943035239049da643 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Tue, 8 Nov 2022 16:30:42 -0800 Subject: [PATCH 3/4] simplify tests --- .../combobox/test/ComboBox.test.js | 88 ++++++++----------- 1 file changed, 35 insertions(+), 53 deletions(-) diff --git a/packages/@react-spectrum/combobox/test/ComboBox.test.js b/packages/@react-spectrum/combobox/test/ComboBox.test.js index c6bab06565a..e4b2be9e994 100644 --- a/packages/@react-spectrum/combobox/test/ComboBox.test.js +++ b/packages/@react-spectrum/combobox/test/ComboBox.test.js @@ -248,8 +248,7 @@ describe('ComboBox', function () { jest.spyOn(window.HTMLElement.prototype, 'clientHeight', 'get').mockImplementation(() => 1000); window.HTMLElement.prototype.scrollIntoView = jest.fn(); jest.spyOn(window.screen, 'width', 'get').mockImplementation(() => 1024); - jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => setTimeout(cb, 0)); - jest.useFakeTimers('legacy'); + jest.useFakeTimers(); }); beforeEach(() => { @@ -2031,8 +2030,9 @@ describe('ComboBox', function () { expect(onLoadMore).toHaveBeenCalledTimes(0); // open menu + triggerPress(button); + // use async act to resolve initial load await act(async () => { - triggerPress(button); // advance to open state from Transition jest.advanceTimersToNextTimer(); }); @@ -2049,30 +2049,19 @@ describe('ComboBox', function () { expect(onLoadMore).toHaveBeenCalledTimes(1); expect(load).toHaveBeenCalledTimes(1); - if (React.version.startsWith('17') || React.version.startsWith('16')) { - // close menu - act(() => { - combobox.blur(); - // on exiting - jest.advanceTimersToNextTimer(); - // raf from virtualizer relayout - jest.advanceTimersToNextTimer(); - // advance to onExited - jest.advanceTimersToNextTimer(); - }); - } else { - // close menu - act(() => { - combobox.blur(); - // raf from virtualizer relayout - jest.advanceTimersToNextTimer(); - }); - // previous act wraps up onExiting - act(() => { - // advance to onExited - jest.advanceTimersToNextTimer(); - }); - } + // close menu + act(() => {combobox.blur();}); + // raf from virtualizer relayout + act(() => {jest.advanceTimersToNextTimer();}); + // previous act wraps up onExiting + // raf + act(() => {jest.advanceTimersToNextTimer();}); + // raf + act(() => {jest.advanceTimersToNextTimer();}); + // exited + act(() => {jest.advanceTimersToNextTimer();}); + + expect(listbox).not.toBeInTheDocument(); }); it('onLoadMore is not called on when previously opened', async () => { @@ -2082,6 +2071,8 @@ describe('ComboBox', function () { ); + let start = +new Date(); + let delta = () => console.log(+new Date() - start); let button = getByRole('button'); let combobox = getByRole('combobox'); expect(load).toHaveBeenCalledTimes(1); @@ -2093,8 +2084,9 @@ describe('ComboBox', function () { clientHeightSpy.mockRestore(); clientHeightSpy = jest.spyOn(window.HTMLElement.prototype, 'clientHeight', 'get').mockImplementationOnce(() => 0).mockImplementation(() => 40); // open menu + triggerPress(button); + // use async act to resolve initial load await act(async () => { - triggerPress(button); // advance to open state from Transition jest.advanceTimersToNextTimer(); }); @@ -2110,30 +2102,17 @@ describe('ComboBox', function () { expect(onOpenChange).toHaveBeenCalledWith(true, 'manual'); expect(onLoadMore).toHaveBeenCalledTimes(1); - if (React.version.startsWith('17') || React.version.startsWith('16')) { - // close menu - act(() => { - combobox.blur(); - // on exiting - jest.advanceTimersToNextTimer(); - // raf from virtualizer relayout - jest.advanceTimersToNextTimer(); - // advance to onExited - jest.advanceTimersToNextTimer(); - }); - } else { - // close menu - act(() => { - combobox.blur(); - // raf from virtualizer relayout - jest.advanceTimersToNextTimer(); - }); - // previous act wraps up onExiting - act(() => { - // advance to onExited - jest.advanceTimersToNextTimer(); - }); - } + // close menu + act(() => {combobox.blur();}); + // raf from virtualizer relayout + act(() => {jest.advanceTimersToNextTimer();}); + // previous act wraps up onExiting + // raf + act(() => {jest.advanceTimersToNextTimer();}); + // raf + act(() => {jest.advanceTimersToNextTimer();}); + // exited + act(() => {jest.advanceTimersToNextTimer();}); expect(listbox).not.toBeInTheDocument(); @@ -2144,8 +2123,8 @@ describe('ComboBox', function () { clientHeightSpy.mockRestore(); clientHeightSpy = jest.spyOn(window.HTMLElement.prototype, 'clientHeight', 'get').mockImplementationOnce(() => 0).mockImplementation(() => 40); // reopen menu + triggerPress(button); await act(async () => { - triggerPress(button); // advance to open state from Transition jest.advanceTimersToNextTimer(); }); @@ -2165,6 +2144,9 @@ describe('ComboBox', function () { // while the test doesn't, causing virtualizer to try to load more expect(onLoadMore).toHaveBeenCalledTimes(2); expect(load).toHaveBeenCalledTimes(1); + + // close menu + act(() => {combobox.blur();}); }); }); From b4a2df3e0ed1538d28181cc02c67bc78f4774f27 Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Tue, 8 Nov 2022 16:31:23 -0800 Subject: [PATCH 4/4] fix lint --- packages/@react-spectrum/combobox/test/ComboBox.test.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/@react-spectrum/combobox/test/ComboBox.test.js b/packages/@react-spectrum/combobox/test/ComboBox.test.js index e4b2be9e994..3c8d4a29388 100644 --- a/packages/@react-spectrum/combobox/test/ComboBox.test.js +++ b/packages/@react-spectrum/combobox/test/ComboBox.test.js @@ -2071,8 +2071,6 @@ describe('ComboBox', function () { ); - let start = +new Date(); - let delta = () => console.log(+new Date() - start); let button = getByRole('button'); let combobox = getByRole('combobox'); expect(load).toHaveBeenCalledTimes(1);