Skip to content

Upgrade React Transition Group #3706

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 8 commits into from
Nov 18, 2022
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
85 changes: 67 additions & 18 deletions packages/@react-spectrum/combobox/test/ComboBox.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down Expand Up @@ -2024,23 +2023,45 @@ 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
triggerPress(button);
// use async act to resolve initial load
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);

// 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();});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we wanting to timers so specifically? Verse jest.runAllTimers()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

each timer causes a state change, all state changes must happen within an act, once the act is done, the state changes are resolved. in this case, the state changes must be resolved separately because they influence what happens to the next timer.

there are still instances where you'd want to potentially advance multiple timers at once, which is what happens in the majority of our tests


expect(listbox).not.toBeInTheDocument();
});

it('onLoadMore is not called on when previously opened', async () => {
Expand All @@ -2052,29 +2073,46 @@ 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
triggerPress(button);
// use async act to resolve initial load
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();
});
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();

expect(onOpenChange).toHaveBeenCalledTimes(2);
expect(onOpenChange).toHaveBeenLastCalledWith(false, undefined);
Expand All @@ -2083,10 +2121,18 @@ 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);
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');
Expand All @@ -2095,7 +2141,10 @@ 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);

// close menu
act(() => {combobox.blur();});
});
});

Expand Down
2 changes: 1 addition & 1 deletion packages/@react-spectrum/list/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
"@react-types/grid": "^3.1.5",
"@react-types/shared": "^3.16.0",
"@spectrum-icons/ui": "^3.4.0",
"react-transition-group": "^2.2.0"
"react-transition-group": "^4.4.5"
},
"devDependencies": {
"@adobe/spectrum-css-temp": "^3.0.0-alpha.1",
Expand Down
2 changes: 1 addition & 1 deletion packages/@react-spectrum/overlays/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
"@react-stately/overlays": "^3.4.3",
"@react-types/overlays": "^3.6.5",
"@react-types/shared": "^3.16.0",
"react-transition-group": "^2.2.0"
"react-transition-group": "^4.4.5"
},
"devDependencies": {
"@adobe/spectrum-css-temp": "3.0.0-alpha.1"
Expand Down
2 changes: 1 addition & 1 deletion packages/@react-spectrum/overlays/src/OpenTransition.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
41 changes: 28 additions & 13 deletions packages/@react-stately/virtualizer/src/useVirtualizerState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,22 +80,37 @@ export function useVirtualizerState<T extends object, V, W>(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(() => ({
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memo state so it doesn't cause unnecessary onLoadMore calls, this appeared after blurring the combobox and it beginning to exit in 16 and 17

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;
}
30 changes: 13 additions & 17 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1125,7 +1125,7 @@
core-js-pure "^3.0.0"
regenerator-runtime "^0.13.4"

"@babel/[email protected]", "@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/[email protected]", "@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==
Expand Down Expand Up @@ -8684,12 +8684,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"
Expand Down Expand Up @@ -18615,11 +18616,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"
Expand Down Expand Up @@ -18653,15 +18649,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"
Expand Down