Skip to content
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

Upgrade support for react 17 #1040

Merged
merged 29 commits into from
Sep 29, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
9593991
[WIP] Upgrade to react 17 support
snowystinger Sep 1, 2020
9d5edcc
remove unneeded chalk patch now that we are using the new version
snowystinger Sep 1, 2020
243e131
fix a ref to the new way of doing things
snowystinger Sep 2, 2020
ee383a7
fix sidenav tests
snowystinger Sep 2, 2020
f144a71
Queue up useId updates to fix bad setState
snowystinger Sep 3, 2020
f3edb0c
Adding a bunch of comments explaining things so that i don't lose my …
snowystinger Sep 3, 2020
319ebc7
make sure to clean up listeners
snowystinger Sep 4, 2020
2b407b8
We can leave these not capturing for now, no tests are failing currently
snowystinger Sep 4, 2020
3b3d8d6
one more comment
snowystinger Sep 4, 2020
a6e41f8
Trying to find the node error stack
snowystinger Sep 4, 2020
fd8d1be
Fixes the remainder of the tests, however, i need to add one now for …
snowystinger Sep 4, 2020
e46728e
put us back on react 16, with allowance for 17, i'll add circlci job …
snowystinger Sep 4, 2020
6a72b6f
reduce updates and fix yarn lock
snowystinger Sep 4, 2020
55cc73f
Got a working scenario for 17
snowystinger Sep 5, 2020
6a569fc
Fix up useId so that it runs immediately if it's not in a render cycle
snowystinger Sep 5, 2020
85ce3a8
fix lint
snowystinger Sep 5, 2020
16052a4
Adding test
snowystinger Sep 14, 2020
fdf17b9
can not put NODE_ENV before the yarn add command, it causes icons to …
snowystinger Sep 14, 2020
4576e75
correct range dep for root package
snowystinger Sep 16, 2020
75409ce
fix yarn lock
snowystinger Sep 16, 2020
9ccf2cf
Increase timeout time so we can pass our tests. Will make issue to ad…
snowystinger Sep 22, 2020
2e1d920
remove unneeded chalk patch now that we are using the new version
snowystinger Sep 23, 2020
3f00c58
remove stale comment
snowystinger Sep 23, 2020
cebffd5
remove more stale comments
snowystinger Sep 23, 2020
4c59490
Merge branch 'main' into pass-react-17-in-jest
dannify Sep 25, 2020
481dbc8
Merge branch 'main' into pass-react-17-in-jest
snowystinger Sep 25, 2020
b70e050
fix syntax error from merge
snowystinger Sep 25, 2020
ed0f7a1
Merge branch 'main' into pass-react-17-in-jest
devongovett Sep 29, 2020
6a00b91
Merge branch 'main' into pass-react-17-in-jest
devongovett Sep 29, 2020
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
Prev Previous commit
Next Next commit
Fixes the remainder of the tests, however, i need to add one now for …
…nested modals

this should check to make sure that the outer one doesn't try to focus inside itself while the inner one is still open
fix lint
  • Loading branch information
snowystinger committed Sep 23, 2020
commit fd8d1be106dc352830d419eadc09195cc5cd2b3b
17 changes: 0 additions & 17 deletions packages/@react-aria/focus/src/FocusScope.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -229,20 +229,6 @@ function useFocusContainment(scopeRef: RefObject<HTMLElement[]>, contain: boolea
};

let onFocus = (e) => {
// I think this onFocus is stealing the focus away from in DialogTrigger test,
// the dialog is closed, at which point focus should be restored to the trigger,
// it would appear that
/**
* dialog is blurred (this is the only blur called that makes it to document)
* dialog is focused (caught from local element listener)
* dialog is focused (global)
* button is focused (global)
* dialog is focused (local)
* dialog is focused (global)
*
* it's fired on so many things so fast that i don't think onFocus actually fires on the button?
*/

// If a focus event occurs outside the active scope (e.g. user tabs from browser location bar),
// restore focus to the previously focused node or the first tabbable element in the active scope.
let isInAnyScope = isElementInAnyScope(e.target, scopes);
Expand Down Expand Up @@ -272,9 +258,6 @@ function useFocusContainment(scopeRef: RefObject<HTMLElement[]>, contain: boolea
};

document.addEventListener('keydown', onKeyDown, false);
/**
* removing the two global listeners fixes dialog trigger, but breaks FocusScope, Dialog containment, and Popover containment
*/
document.addEventListener('focusin', onFocus, false);
scope.forEach(element => element.addEventListener('focusin', onFocus, false));
scope.forEach(element => element.addEventListener('focusout', onBlur, false));
Expand Down
7 changes: 1 addition & 6 deletions packages/@react-aria/interactions/src/useFocusVisible.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,12 +134,7 @@ function setupGlobalFocusEvents() {
// Register focus events on the window so they are sure to happen
// before React's event listeners (registered on the document).
window.addEventListener('focus', handleFocusEvent, true);
window.addEventListener('blur', handleWindowBlur, true); // reverting this fixes the Picker test that's failing
/**
* it's happening because as a capture, it's getting a window blur when it shouldn't, when the spectrum-Menu is focused
* I assume because of the blur on the trigger?
* also, this is getting fired a LOT
*/
window.addEventListener('blur', handleWindowBlur, false);

if (typeof PointerEvent !== 'undefined') {
document.addEventListener('pointerdown', handlePointerEvent, true);
Expand Down
17 changes: 3 additions & 14 deletions packages/@react-aria/utils/src/useId.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@ let map: Map<string, (v: string) => void> = new Map();
*/
export function useId(defaultId?: string): string {
let [value, setValue] = useState(defaultId);
// do i need to keep an entire queue? i think I only need the most recent?
// does this actually work in the case that we're not mid render? it's a ref, so it won't
// Do i need to keep an entire queue? i think I only need the most recent?
// Does this actually work in the case that we're not mid render? It's a ref, so it won't
// cause a re-render ever on it's own, that said, we might always be mid-render
// we seem to only call mergeProps/Ids in the render flow, but now we'll need to document that
// as a limitation of those functions
// as a limitation of those functions.
let setIdUpdateQueue = useRef([]);
let pushToQueue = useCallback((val) => {
setIdUpdateQueue.current.push(val);
Expand All @@ -43,17 +43,6 @@ export function useId(defaultId?: string): string {
return res;
}

// TODO this causes issues
/**
* In updating to react 17, calling setState from another component gets more complex, however, we do this pretty easily/accidentally
mergeProps calls on mergeIds which calls setState
we frequently call mergeProps during the render cycle
that setState isn’t necessarily the state of the component rendering, ergo, bad setState call
anyone have any ideas how to resolve this?
technically we’d need to make that setState call behind a useEffeect, but mergeId’s isn’t a hook… so no useEffect
we could put it around mergeProps, but that seems complicated and impossible to enforce
o, and i can’t make mergeId’s a hook either, that’d be breaking i think
*/
/**
* Merges two ids.
*/
Expand Down
12 changes: 0 additions & 12 deletions packages/@react-spectrum/button/stories/Button.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import {Flex} from '@react-spectrum/layout';
import React from 'react';
import {storiesOf} from '@storybook/react';
import {Text} from '@react-spectrum/text';
import {FocusScope} from "@react-aria/focus";

storiesOf('Button', module)
.addParameters({providerSwitcher: {status: 'positive'}})
Expand Down Expand Up @@ -89,17 +88,6 @@ storiesOf('Button', module)
.add(
'element: a, rel: \'noopener noreferrer\'',
() => render({elementType: 'a', href: '//example.com', rel: 'noopener noreferrer', variant: 'primary'})
)
.add(
'focus scope',
() => (<div>
<input data-testid="outside" />
<FocusScope contain>
<input data-testid="input1" />
<input data-testid="input2" />
<input data-testid="input3" />
</FocusScope>
</div>)
);

function render(props: any = {}) {
Expand Down
34 changes: 17 additions & 17 deletions packages/@react-spectrum/datepicker/test/DatePicker.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
* governing permissions and limitations under the License.
*/

import {act, fireEvent, render} from '@testing-library/react';
import {DatePicker} from '../';
import {fireEvent, render} from '@testing-library/react';
import {Provider} from '@react-spectrum/provider';
import React from 'react';
import {theme} from '@react-spectrum/theme-default';
Expand Down Expand Up @@ -275,7 +275,7 @@ describe('DatePicker', function () {
let {getByLabelText, unmount} = render(<DatePicker value={value} onChange={onChange} formatOptions={format} />);
let segment = getByLabelText(label);
let textContent = segment.textContent;
segment.focus();
act(() => {segment.focus();});

fireEvent.keyDown(segment, {key: options.upKey || 'ArrowUp'});
expect(onChange).toHaveBeenCalledTimes(1);
Expand All @@ -293,7 +293,7 @@ describe('DatePicker', function () {
({getByLabelText, unmount} = render(<DatePicker defaultValue={value} onChange={onChange} formatOptions={format} />));
segment = getByLabelText(label);
textContent = segment.textContent;
segment.focus();
act(() => {segment.focus();});

fireEvent.keyDown(segment, {key: options.upKey || 'ArrowUp'});
expect(onChange).toHaveBeenCalledTimes(1);
Expand All @@ -306,7 +306,7 @@ describe('DatePicker', function () {
({getByLabelText, unmount} = render(<DatePicker defaultValue={value} onChange={onChange} formatOptions={format} />));
segment = getByLabelText(label);
textContent = segment.textContent;
segment.focus();
act(() => {segment.focus();});

fireEvent.keyDown(segment, {key: options.downKey || 'ArrowDown'});
expect(onChange).toHaveBeenCalledTimes(1);
Expand All @@ -319,7 +319,7 @@ describe('DatePicker', function () {
({getByLabelText, unmount} = render(<DatePicker defaultValue={value} isReadOnly onChange={onChange} formatOptions={format} />));
segment = getByLabelText(label);
textContent = segment.textContent;
segment.focus();
act(() => {segment.focus();});

fireEvent.keyDown(segment, {key: options.upKey || 'ArrowUp'});
expect(onChange).not.toHaveBeenCalled();
Expand All @@ -331,7 +331,7 @@ describe('DatePicker', function () {
({getByLabelText, unmount} = render(<DatePicker defaultValue={value} isReadOnly onChange={onChange} formatOptions={format} />));
segment = getByLabelText(label);
textContent = segment.textContent;
segment.focus();
act(() => {segment.focus();});

fireEvent.keyDown(segment, {key: options.downKey || 'ArrowDown'});
expect(onChange).not.toHaveBeenCalled();
Expand Down Expand Up @@ -488,7 +488,7 @@ describe('DatePicker', function () {
let {getByLabelText, getAllByRole, unmount} = render(<DatePicker value={value} onChange={onChange} formatOptions={format} />);
let segment = getByLabelText(label);
let textContent = segment.textContent;
segment.focus();
act(() => {segment.focus();});

let i = 0;
for (let key of keys) {
Expand Down Expand Up @@ -518,7 +518,7 @@ describe('DatePicker', function () {
({getByLabelText, getAllByRole, unmount} = render(<DatePicker defaultValue={value} onChange={onChange} formatOptions={format} />));
segment = getByLabelText(label);
textContent = segment.textContent;
segment.focus();
act(() => {segment.focus();});

i = 0;
for (let key of keys) {
Expand Down Expand Up @@ -548,7 +548,7 @@ describe('DatePicker', function () {
({getByLabelText, getAllByRole, unmount} = render(<DatePicker defaultValue={value} isReadOnly onChange={onChange} formatOptions={format} />));
segment = getByLabelText(label);
textContent = segment.textContent;
segment.focus();
act(() => {segment.focus();});

for (let key of keys) {
fireEvent.keyDown(segment, {key});
Expand Down Expand Up @@ -641,7 +641,7 @@ describe('DatePicker', function () {
let {getByLabelText, unmount} = render(<DatePicker value={value} onChange={onChange} formatOptions={format} />);
let segment = getByLabelText(label);
let textContent = segment.textContent;
segment.focus();
act(() => {segment.focus();});

fireEvent.keyDown(segment, {key: 'Backspace'});
expect(onChange).toHaveBeenCalledTimes(1);
Expand All @@ -654,7 +654,7 @@ describe('DatePicker', function () {
({getByLabelText, unmount} = render(<DatePicker defaultValue={value} onChange={onChange} formatOptions={format} />));
segment = getByLabelText(label);
textContent = segment.textContent;
segment.focus();
act(() => {segment.focus();});

fireEvent.keyDown(segment, {key: 'Backspace'});
expect(onChange).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -716,7 +716,7 @@ describe('DatePicker', function () {
);
let segment = getByLabelText('العام');
expect(segment).toHaveTextContent('٢٠١٩');
segment.focus();
act(() => {segment.focus();});

fireEvent.keyDown(segment, {key: 'Backspace'});
expect(onChange).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -797,7 +797,7 @@ describe('DatePicker', function () {
expect(combobox).toHaveTextContent(`1/1/${new Date().getFullYear()}`);

let segments = getAllByRole('spinbutton');
segments[0].focus();
act(() => {segments[0].focus();});

fireEvent.keyDown(document.activeElement, {key: 'Enter'});
expect(segments[1]).toHaveFocus();
Expand All @@ -822,7 +822,7 @@ describe('DatePicker', function () {
expect(combobox).toHaveTextContent(`1/1/${new Date().getFullYear()}`);

let segments = getAllByRole('spinbutton');
segments[0].focus();
act(() => {segments[0].focus();});

fireEvent.keyDown(document.activeElement, {key: 'ArrowUp'});
fireEvent.keyDown(document.activeElement, {key: 'ArrowRight'});
Expand Down Expand Up @@ -850,7 +850,7 @@ describe('DatePicker', function () {
expect(combobox).toHaveTextContent(`1/1/${new Date().getFullYear()}`);

let segments = getAllByRole('spinbutton');
segments[0].focus();
act(() => {segments[0].focus();});

fireEvent.keyDown(document.activeElement, {key: 'ArrowUp'});
fireEvent.keyDown(document.activeElement, {key: 'ArrowRight'});
Expand Down Expand Up @@ -881,7 +881,7 @@ describe('DatePicker', function () {
expect(combobox).toHaveTextContent(`1/1/${new Date().getFullYear()}`);

let segments = getAllByRole('spinbutton');
segments[0].focus();
act(() => {segments[0].focus();});

fireEvent.keyDown(document.activeElement, {key: '4'});
expect(segments[1]).toHaveFocus();
Expand Down Expand Up @@ -914,7 +914,7 @@ describe('DatePicker', function () {
expect(combobox).toHaveTextContent(`1/1/${new Date().getFullYear()}`);

let segments = getAllByRole('spinbutton');
segments[0].focus();
act(() => {segments[0].focus();});

fireEvent.keyDown(document.activeElement, {key: '4'});
expect(segments[1]).toHaveFocus();
Expand Down
11 changes: 8 additions & 3 deletions packages/@react-spectrum/dialog/test/DialogTrigger.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {Provider} from '@react-spectrum/provider';
import React from 'react';
import {theme} from '@react-spectrum/theme-default';
import {triggerPress} from '@react-spectrum/test-utils';
import userEvent from "@testing-library/user-event";
import userEvent from '@testing-library/user-event';

// whole thing hangs if run with rest of suite, but doesn't on its own
describe('DialogTrigger', function () {
Expand All @@ -33,7 +33,8 @@ describe('DialogTrigger', function () {

beforeEach(() => {
matchMedia = new MatchMediaMock();
jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => cb());
// this needs to be a setTimeout so that the dialog can be removed from the dom before the callback is invoked
jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => setTimeout(() => cb(), 0));
});

afterEach(() => {
Expand Down Expand Up @@ -239,7 +240,7 @@ describe('DialogTrigger', function () {
let button = getByRole('button');
act(() => {button.focus();});
fireEvent.focusIn(button);
act(() => {userEvent.click(button)});
act(() => {userEvent.click(button);});

act(() => {
jest.runAllTimers();
Expand All @@ -264,6 +265,10 @@ describe('DialogTrigger', function () {
expect(dialog).not.toBeInTheDocument();
}); // wait for animation

act(() => {
jest.runAllTimers();
});

expect(document.activeElement).toBe(button);
});

Expand Down
2 changes: 1 addition & 1 deletion packages/@react-spectrum/toast/test/Toast.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
* governing permissions and limitations under the License.
*/

import React from 'react';
import {act, render} from '@testing-library/react';
import React from 'react';
import {Toast} from '../';
import {triggerPress} from '@react-spectrum/test-utils';
import {Toast as V2Toast} from '@react/react-spectrum/Toast';
Expand Down