-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Refactor some components to match the react-hooks/exhaustive-deps eslint rules
#25064
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -146,6 +146,16 @@ _Returns_ | |
|
|
||
| - `boolean`: Whether or not the text has been copied. Resets after the timeout. | ||
|
|
||
| <a name="useDidMount" href="#useDidMount">#</a> **useDidMount** | ||
|
|
||
| A drop-in replacement of the hook version of `componentDidMount`. | ||
| Like `useEffect` but only called once when the component is mounted. | ||
| This hook is only used for backward-compatibility reason. Consider using `useEffect` wherever possible. | ||
|
|
||
| _Parameters_ | ||
|
|
||
| - _effect_ `Function`: The effect callback passed to `useEffect`. | ||
|
|
||
| <a name="useInstanceId" href="#useInstanceId">#</a> **useInstanceId** | ||
|
|
||
| Provides a unique instance ID. | ||
|
|
@@ -165,6 +175,22 @@ _Parameters_ | |
| - _callback_ `Function`: Shortcut callback. | ||
| - _options_ `WPKeyboardShortcutConfig`: Shortcut options. | ||
|
|
||
| <a name="useLazyRef" href="#useLazyRef">#</a> **useLazyRef** | ||
|
Contributor
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. That one is a bit confusing, it's very unclear what it does and when to use it. Makes me wonder if it deserves to be a public API on the compose package.
Member
Author
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. In my own experience, I find This hook is basically giving useState(() => expensiveCalculation())[0]; // returns a value
useLazyRef(() => expensiveCalculation()); // returns a ref
Member
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. "Lazy" sounds a bit weird. Could we call it
Member
Author
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. It's not memoized though, and it's also not the same as
Member
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. Sounds good. How can we make it clearer that the laziness only applies to initial value?
Member
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. Makes me wonder if there's ever a use case for the initial ref to be set on every render...
Member
Author
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. Nope, not that I can think of 😅. Ref is always only be set on the initial render, it's like the instance variable of class components.
Member
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. Right, so I wonder why we don't do this by default (or why React doesn't).
Member
Author
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. I think it's because that function initializer still adds some overheads to the component, even though it's really small, it will still be used by a large number of projects. |
||
|
|
||
| Like `useRef` but only run the initializer once. | ||
|
|
||
| _Parameters_ | ||
|
|
||
| - _initializer_ `Function`: A function to return the ref object. | ||
|
|
||
| _Returns_ | ||
|
|
||
| - `MutableRefObject`: The returned ref object. | ||
|
|
||
| _Type Definition_ | ||
|
|
||
| - _MutableRefObject_ (unknown type) | ||
|
|
||
| <a name="useMediaQuery" href="#useMediaQuery">#</a> **useMediaQuery** | ||
|
|
||
| Runs a media query and returns its value when it changes. | ||
|
|
@@ -222,6 +248,17 @@ _Returns_ | |
|
|
||
| - `Array`: An array of {Element} `resizeListener` and {?Object} `sizes` with properties `width` and `height` | ||
|
|
||
| <a name="useShallowCompareEffect" href="#useShallowCompareEffect">#</a> **useShallowCompareEffect** | ||
|
|
||
| Like `useEffect` but call the effect when the dependencies are not shallowly equal. | ||
| Useful when the size of the dependency array might change during re-renders. | ||
| This hook is only used for backward-compatibility reason. Consider using `useEffect` wherever possible. | ||
|
|
||
| _Parameters_ | ||
|
|
||
| - _effect_ `Function`: The effect callback passed to `useEffect`. | ||
| - _deps_ `Array`: The dependency array that is compared against shallowly. | ||
|
|
||
| <a name="useViewportMatch" href="#useViewportMatch">#</a> **useViewportMatch** | ||
|
|
||
| Returns true if the viewport matches the given query, or false otherwise. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { useLayoutEffect, useRef } from '@wordpress/element'; | ||
|
|
||
| /** | ||
| * A drop-in replacement of the hook version of `componentDidMount`. | ||
| * Like `useEffect` but only called once when the component is mounted. | ||
| * This hook is only used for backward-compatibility reason. Consider using `useEffect` wherever possible. | ||
| * | ||
| * @param {Function} effect The effect callback passed to `useEffect`. | ||
| */ | ||
| function useDidMount( effect ) { | ||
|
Member
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. I think
Member
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. Perhaps we should also create two versions of this hook... one for
Member
Author
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. I disagree though, 99% of cases we can just use Same thing goes for creating two versions of this hook. We can always create them if necessary. I find these hooks more like utility hooks we can share between packages, but not like a standalone hooks library that's going to cover all cases. That's just my take though, happy to be proven wrong :)!
Member
Author
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. Just want to share this tweet from React core team to back up my reasoning that making
Member
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. Thanks. Another React dev tweet worth sharing: https://twitter.com/dan_abramov/status/1284510166503890944 |
||
| const effectRef = useRef( effect ); | ||
| effectRef.current = effect; | ||
|
|
||
| // `useLayoutEffect` because that's closer to how the `componentDidMount` works. | ||
| useLayoutEffect( () => effectRef.current(), [] ); | ||
| } | ||
|
|
||
| export default useDidMount; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| /** | ||
| * External dependencies | ||
| */ | ||
| import { render } from '@testing-library/react'; | ||
|
|
||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import React, { Component, useEffect } from '@wordpress/element'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import useDidMount from '../'; | ||
|
|
||
| describe( 'useDidMount', () => { | ||
| it( 'should call the effect when did mount', () => { | ||
| const mountEffect = jest.fn(); | ||
|
|
||
| function TestComponent() { | ||
| useDidMount( mountEffect ); | ||
| return null; | ||
| } | ||
|
|
||
| render( <TestComponent /> ); | ||
|
|
||
| expect( mountEffect ).toHaveBeenCalledTimes( 1 ); | ||
| } ); | ||
|
|
||
| it( 'should call the cleanup function when unmount', () => { | ||
| const unmountCallback = jest.fn(); | ||
|
|
||
| function TestComponent() { | ||
| useDidMount( () => unmountCallback ); | ||
| return null; | ||
| } | ||
|
|
||
| const { unmount } = render( <TestComponent /> ); | ||
|
|
||
| expect( unmountCallback ).not.toHaveBeenCalled(); | ||
|
|
||
| unmount(); | ||
|
|
||
| expect( unmountCallback ).toHaveBeenCalledTimes( 1 ); | ||
| } ); | ||
|
|
||
| it( 'should match the calling order of componentDidMount', async () => { | ||
| const mountEffectCallback = jest.fn(); | ||
| const effectCallback = jest.fn(); | ||
|
|
||
| const didMountCallback = jest.fn( | ||
| () => | ||
| new Promise( ( resolve ) => { | ||
| expect( mountEffectCallback ).toHaveBeenCalled(); | ||
| expect( effectCallback ).not.toHaveBeenCalled(); | ||
|
|
||
| resolve(); | ||
| } ) | ||
| ); | ||
|
|
||
| let promise; | ||
|
|
||
| class DidMount extends Component { | ||
| componentDidMount() { | ||
| promise = didMountCallback(); | ||
| } | ||
| render() { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| function Hook() { | ||
| useDidMount( mountEffectCallback ); | ||
| useEffect( effectCallback ); | ||
| return null; | ||
| } | ||
|
|
||
| function TestComponent() { | ||
| return ( | ||
| <> | ||
| <Hook /> | ||
| <DidMount /> | ||
| </> | ||
| ); | ||
| } | ||
|
|
||
| render( <TestComponent /> ); | ||
|
|
||
| await promise; | ||
| } ); | ||
| } ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { useRef } from '@wordpress/element'; | ||
|
|
||
| const INITIAL_TAG = Symbol( 'INITIAL_TAG' ); | ||
|
|
||
| /** | ||
| * Like `useRef` but only run the initializer once. | ||
| * | ||
| * @typedef {import('@types/react').MutableRefObject} MutableRefObject | ||
| * | ||
| * @param {Function} initializer A function to return the ref object. | ||
| * @return {MutableRefObject} The returned ref object. | ||
| */ | ||
| function useLazyRef( initializer ) { | ||
|
Member
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. Why not use
Member
Author
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.
|
||
| const ref = useRef( INITIAL_TAG ); | ||
|
|
||
| if ( ref.current === INITIAL_TAG ) { | ||
| ref.current = initializer(); | ||
| } | ||
|
|
||
| return ref; | ||
| } | ||
|
|
||
| export default useLazyRef; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import React, { useReducer } from '@wordpress/element'; | ||
|
|
||
| /** | ||
| * External dependencies | ||
| */ | ||
| import { render, act } from '@testing-library/react'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import useLazyRef from '..'; | ||
|
|
||
| describe( 'useLazyRef', () => { | ||
| it( 'should lazily initialize the initializer only once', () => { | ||
| const initializer = jest.fn( () => 87 ); | ||
| let result; | ||
| let forceUpdate = () => {}; | ||
|
|
||
| function TestComponent() { | ||
| const ref = useLazyRef( initializer ); | ||
|
|
||
| forceUpdate = useReducer( ( c ) => c + 1, 0 )[ 1 ]; | ||
|
|
||
| result = ref.current; | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| render( <TestComponent /> ); | ||
|
|
||
| expect( initializer ).toHaveBeenCalledTimes( 1 ); | ||
| expect( result ).toBe( 87 ); | ||
|
|
||
| act( () => { | ||
| forceUpdate(); | ||
| } ); | ||
|
|
||
| expect( initializer ).toHaveBeenCalledTimes( 1 ); | ||
| expect( result ).toBe( 87 ); | ||
| } ); | ||
|
|
||
| it( 'should not accept falsy values', () => { | ||
| const initializer = jest.fn( () => 87 ); | ||
| let result; | ||
| let ref = { current: null }; | ||
| let forceUpdate = () => {}; | ||
|
|
||
| function TestComponent() { | ||
| ref = useLazyRef( initializer ); | ||
|
|
||
| forceUpdate = useReducer( ( c ) => c + 1, 0 )[ 1 ]; | ||
|
|
||
| result = ref.current; | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| render( <TestComponent /> ); | ||
|
|
||
| expect( initializer ).toHaveBeenCalledTimes( 1 ); | ||
| expect( result ).toBe( 87 ); | ||
|
|
||
| ref.current = undefined; | ||
| act( () => { | ||
| forceUpdate(); | ||
| } ); | ||
|
|
||
| expect( initializer ).toHaveBeenCalledTimes( 1 ); | ||
| expect( result ).toBe( undefined ); | ||
|
|
||
| ref.current = null; | ||
| act( () => { | ||
| forceUpdate(); | ||
| } ); | ||
|
|
||
| expect( initializer ).toHaveBeenCalledTimes( 1 ); | ||
| expect( result ).toBe( null ); | ||
| } ); | ||
| } ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { useEffect, useRef } from '@wordpress/element'; | ||
| import isShallowEqual from '@wordpress/is-shallow-equal'; | ||
|
|
||
| /** | ||
| * Like `useEffect` but call the effect when the dependencies are not shallowly equal. | ||
| * Useful when the size of the dependency array might change during re-renders. | ||
| * This hook is only used for backward-compatibility reason. Consider using `useEffect` wherever possible. | ||
| * | ||
| * @param {Function} effect The effect callback passed to `useEffect`. | ||
| * @param {Array} deps The dependency array that is compared against shallowly. | ||
| */ | ||
| function useShallowCompareEffect( effect, deps ) { | ||
|
Contributor
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 hook is a bit confusing what's the difference between doing this and spreading
Member
Author
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. The main difference is that in the original effect, we are passing a dependency array dynamically, which means the size of the array could change. When it changes, it will throw a warning from React like below.
This hook hack around this by putting them inside an item in the dependency array, so that even when the size changed, it won't throw warnings at us. |
||
| const ref = useRef(); | ||
|
|
||
| if ( ! isShallowEqual( ref.current, deps ) ) { | ||
| ref.current = deps; | ||
| } | ||
|
|
||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| useEffect( effect, [ ref.current ] ); | ||
| } | ||
|
|
||
| export default useShallowCompareEffect; | ||
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.
This is basically the same thing, too bad that we're forced to do that due to the lint rule.
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.
Yeah, it's a bummer, we can create a custom hook for this, but I doubt that it's worth it. It doesn't actually hurt to just write it like this anyway, at least it makes the compiler happy 😅
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.
Are we forced to use that rule?
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.
That's the whole point of this PR though 😅. IMO, The benefits outweighs the inconveniences.
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.
If we wanted to, we could use a
/* eslint-disable RULE-NAME */for this particular use-case, with a comment explaining why.