-
Notifications
You must be signed in to change notification settings - Fork 1.3k
chore: Update eslint plugin react hooks followup #9054
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
base: main
Are you sure you want to change the base?
Changes from all commits
5d3e2f3
7b96414
7f79dd9
1208e74
3f9b6c9
116fe27
a7d4e7e
30bc1fa
5a45466
ba77f40
8f1cd8e
95687e9
a5803c0
be90742
4cd669d
c073772
2f591d5
500c564
a81fa41
bad9964
e88adfa
fe3d7b5
37dee08
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,7 @@ | |
| */ | ||
|
|
||
| import {mergeRefs} from '../'; | ||
| import React, {useCallback, useRef} from 'react'; | ||
| import React, {useCallback, useEffect, useRef} from 'react'; | ||
| import {render} from '@react-spectrum/test-utils-internal'; | ||
|
|
||
| describe('mergeRefs', () => { | ||
|
|
@@ -20,16 +20,21 @@ describe('mergeRefs', () => { | |
| let ref2; | ||
|
|
||
| const TextField = (props) => { | ||
| ref1 = useRef(null); | ||
| ref2 = useRef(null); | ||
| let internalRef1 = useRef(null); | ||
| let internalRef2 = useRef(null); | ||
| useEffect(() => { | ||
| ref1 = internalRef1; | ||
| ref2 = internalRef2; | ||
| }, [internalRef1, internalRef2]); | ||
|
|
||
| const ref = mergeRefs(ref1, ref2); | ||
| const ref = mergeRefs(internalRef1, internalRef2); | ||
| return <input {...props} ref={ref} />; | ||
| }; | ||
|
|
||
| render(<TextField foo="foo" />); | ||
|
|
||
| expect(ref1.current).toBe(ref2.current); | ||
| expect(ref1.current).not.toBeNull(); | ||
| }); | ||
|
|
||
| if (parseInt(React.version.split('.')[0], 10) >= 19) { | ||
|
|
@@ -40,14 +45,18 @@ describe('mergeRefs', () => { | |
| let target = null; | ||
|
|
||
| const TextField = (props) => { | ||
| ref1 = useRef(null); | ||
| ref2 = useRef(null); | ||
| let internalRef1 = useRef(null); | ||
| let internalRef2 = useRef(null); | ||
| useEffect(() => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. other option i guess would be to exclude the compiler from running on test files, but seems better to always be in good habits |
||
| ref1 = internalRef1; | ||
| ref2 = internalRef2; | ||
| }, [internalRef1, internalRef2]); | ||
| let ref3 = useCallback((node) => { | ||
| target = node; | ||
| return cleanUp; | ||
| }, []); | ||
|
|
||
| const ref = mergeRefs(ref1, ref2, ref3); | ||
| const ref = mergeRefs(internalRef1, internalRef2, ref3); | ||
| return <input {...props} ref={ref} />; | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -288,7 +288,6 @@ function ForwardSearchAutocompleteInput<T>(props: SearchAutocompleteInputProps<T | |
| } | ||
| } else if (!isLoading) { | ||
| // If loading is no longer happening, clear any timers and hide the loading circle | ||
| setShowLoading(false); | ||
| if (timeout.current != null) { | ||
| clearTimeout(timeout.current); | ||
| timeout.current = null; | ||
|
|
@@ -297,6 +296,11 @@ function ForwardSearchAutocompleteInput<T>(props: SearchAutocompleteInputProps<T | |
|
|
||
| lastInputValue.current = inputValue; | ||
| }, [isLoading, showLoading, inputValue]); | ||
| let [prevIsLoading, setPrevIsLoading] = useState(isLoading); | ||
| if (prevIsLoading !== isLoading && !isLoading) { | ||
| setShowLoading(false); | ||
| setPrevIsLoading(isLoading); | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah this one felt weird to me as well, but https://react.dev/reference/react/useState#storing-information-from-previous-renders claims it is better than setting state in an effect (which the linter will now complain about) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this one is very annoying, but I'm not sure a better way to do it, we "react" to the change in the prop value, which means we set state in render we use the showLoading state and count on the new render as a response to the timeout up above, so we need to have some way to "un set it" had to do it in Combobox as well |
||
|
|
||
| return ( | ||
| (<FocusRing | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a lot of these, i just moved the random generator out of the component, that should be fine because you couldn't drag between different examples this way either and I don't think we render the same one twice as an example