Refactor some components to match the react-hooks/exhaustive-deps eslint rules
#25064
Conversation
kevin940726
added
the
[Type] Code Quality
|
Size Change: +272 B (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for starting these efforts. RichText is one of the most complex components we have. I'd appreciate @ellatrix's eyes here.
| blockType.title, | ||
| wrapperProps, | ||
| ] | ||
| ); |
There was a problem hiding this comment.
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.
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.
Are we forced to use that rule?
There was a problem hiding this comment.
That's the whole point of this PR though 😅. IMO, The benefits outweighs the inconveniences.
There was a problem hiding this comment.
If we wanted to, we could use a /* eslint-disable RULE-NAME */ for this particular use-case, with a comment explaining why.
| @@ -165,6 +175,22 @@ _Parameters_ | |||
| - _callback_ `Function`: Shortcut callback. | |||
| - _options_ `WPKeyboardShortcutConfig`: Shortcut options. | |||
|
|
|||
| <a name="useLazyRef" href="#useLazyRef">#</a> **useLazyRef** | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
In my own experience, I find useLazyRef very useful in many places though. Whenever we want to create a ref, but only want to create it lazily once, this hook is the way to go.
This hook is basically giving useRef the same super power of useState function initialization.
useState(() => expensiveCalculation())[0]; // returns a value
useLazyRef(() => expensiveCalculation()); // returns a refThere was a problem hiding this comment.
"Lazy" sounds a bit weird. Could we call it useMemoizedRef or something like that? I don't see much benefit though. useRef( useMemo() ) is the same thing?
There was a problem hiding this comment.
It's not memoized though, and it's also not the same as useRef(useMemo()). It only lazily initialize once in the first render.
The name is inspired by this tweet by Dan
There was a problem hiding this comment.
Sounds good. How can we make it clearer that the laziness only applies to initial value?
There was a problem hiding this comment.
Makes me wonder if there's ever a use case for the initial ref to be set on every render...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Right, so I wonder why we don't do this by default (or why React doesn't).
There was a problem hiding this comment.
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. useRef(value) is also way simpler as a public API too.
| * @param {Function} effect The effect callback passed to `useEffect`. | ||
| * @param {Array} deps The dependency array that is compared against shallowly. | ||
| */ | ||
| function useShallowCompareEffect( effect, deps ) { |
There was a problem hiding this comment.
This hook is a bit confusing what's the difference between doing this and spreading deps in the dependencies?
There was a problem hiding this comment.
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.
Warning: The final argument passed to useEffect changed size between renders. The order and size of this array must remain constant.
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.
| @@ -137,6 +135,63 @@ function fixPlaceholderSelection( defaultView ) { | |||
| selection.collapseToStart(); | |||
| } | |||
|
|
|||
| function useReapply( { | |||
There was a problem hiding this comment.
What's the purpose of splitting this out? What rule is it satisfying?
There was a problem hiding this comment.
Just want to group them all together so that it's clear that they are for the same logic.
|
|
||
| shouldReapplyRef.current = false; | ||
| } | ||
| } ); |
There was a problem hiding this comment.
It's actually intended. No dependencies means that the effect will run after every render, so that we don't have to add applyFromProps to the dependency array. applyFromProps changes almost every render anyway, so it basically has the same effect.
There was a problem hiding this comment.
In that case it might be good to comment, since missing dependencies rare and others might be wondering if it's intentional or a mistake.
There was a problem hiding this comment.
What's wrong with calling applyFromProps() from the other effects?
There was a problem hiding this comment.
Yeah, a comment would be helpful!
If we want to call applyFromProps in other effects, it has to be in the dependency array of that effect. However, since applyFromProps almost always changes, the effect will always fire too. One solution would be to split them up into different effects and register another effect to run applyFromProps (like this solution). Another solution would be to rewrite them to better match the mental modal of hooks, but that's more work and could potentially be a breaking change if we're not careful.
| * @param {Function} initializer A function to return the ref object. | ||
| * @return {MutableRefObject} The returned ref object. | ||
| */ | ||
| function useLazyRef( initializer ) { |
There was a problem hiding this comment.
Why not use useRef( useMemo() ) everywhere?
There was a problem hiding this comment.
useMemo has to be passed with dependencies array, and also it's not guaranteed to be run only once.
kevin940726
requested review from
ajitbohra,
cameronvoell,
etoledom and
guarani
as code owners
| * | ||
| * @param {Function} effect The effect callback passed to `useEffect`. | ||
| */ | ||
| function useDidMount( effect ) { |
There was a problem hiding this comment.
I think useMountEffect or useEffectOnMount would be more descriptive names for this hook. I also don't think we should say it's "only used for backward-compatibility reason". There are plenty of valid use-cases to run something only once upon mount.
There was a problem hiding this comment.
Perhaps we should also create two versions of this hook... one for useLayoutEffect and one of useEffect. E.g. useMountLayoutEffect or useLayoutEffectOnMount. (I think the latter reads more clearly.)
There was a problem hiding this comment.
I disagree though, 99% of cases we can just use useEffect with an empty dependency array to achieve the same thing. If we're writing a new component, then I believe we can avoid using this hook if we can "think in hooks" in most cases. Besides, we can always adjust the wording if it does become more and more used!
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 :)!
There was a problem hiding this comment.
Just want to share this tweet from React core team to back up my reasoning that making useMount more exposed might not be a good idea: https://twitter.com/sebmarkbage/status/1313842495256748038?s=21
There was a problem hiding this comment.
Thanks. Another React dev tweet worth sharing: https://twitter.com/dan_abramov/status/1284510166503890944
Description
As per #24914, we try to refactor some components to match the
react-hooks/exhaustive-depseslint rules to compare the outcome and efforts.Refactored
<BlockListBlock>and<RichText>to demonstrate the efforts.<RichText>is tricky, the original style does not match the mental model of hooks, hence lots of violation to the eslint rules. To preserve backward-compatibility and to prevent ground-up rewrites, I chose to introduce three utility hooks in@wordpress/composeto help us migrate to adopt the eslint rules. By far, it's the most difficult component to refactor, others seem pretty straightforward like<BlockListBlock>.How has this been tested?
npm run test-unitTypes of changes
Bug fix
Checklist: