Refactor the popover component using React Hooks

[youknowriad]

The Popover component is also one of our most complex components and refactoring to hooks bring clarity to this component and increases maintainability. I think there are still some improvements we could make but in this version, I tried keeping the logic very similar to what we have currently in the class component.

[youknowriad]

cc @nerrad in case you have an idea why the unit tests are failing :)

[youknowriad]

TestUtils don't work with functional components. I think it's the same reason the dropdown asserters were failing.

[gziolo]

How about using this new util for hooks? I remembered they had introduced something:
https://reactjs.org/docs/hooks-faq.html#how-to-test-components-that-use-hooks

[youknowriad]

Which util? We're using this one already

[nerrad]

I think @gziolo is referring to https://reactjs.org/docs/test-utils.html#act

[youknowriad]

Right! I'm using it here https://github.com/WordPress/gutenberg/pull/15053/files#diff-829fd588eade3365fbb33ef908ef9fe8R41

[nerrad]

Sidenote: Via the docs for the above, there is mention of this library. Looks kind of interesting.

[gziolo]

Sidenote: Via the docs for the above, there is mention of this library. Looks kind of interesting.

We still have over 80 tests using Enzyme 🤷‍♂️We can investigate, but all those abstractions need to make our life easier and ensure we don't run in the same situation like with Enzyme when they are always behind with upgrading when a new version of React comes out.

[nerrad]

Ya I agree. I only referenced the other library because it supports the latest in React and on the surface it looks like they're doing a much better job than enzyme of keeping up to date. I'll probably experiment with the library in my own projects for a bit.

[youknowriad]

@aduth @ellatrix I noticed that in master, there are two new commits tweaking the Popover component after I didi the refactorings. These changes makes it a bit hard for me to rebase. Do you think you could check how to fit this in the hooks implementation? There's no urgency in this PR though

[aduth]

@aduth @ellatrix I noticed that in master, there are two new commits tweaking the Popover component after I didi the refactorings. These changes makes it a bit hard for me to rebase. Do you think you could check how to fit this in the hooks implementation? There's no urgency in this PR though

For reference, the commits:

• a9a3907
• e246157
• 6a26e36

I was looking to rebase locally, and it seemed like you'd already accounted for the anchorRect prop from 6a26e36 ? To me, that would have been the most challenging one. The main gist of it is that the component should defer to anchorRect whenever available, and only schedule the setInterval if and only if anchorRect is not provided.

Looking at the branch in its current form, I think we might need to be considering somewhere hereabounds to cancel and schedule the interval in response to a changing availability of anchorRect:

gutenberg/packages/components/src/popover/index.js

Lines 99 to 110 in a8ffaa3

	useEffect( () => {
	/*
	* There are sometimes we need to reposition or resize the popover that are not
	* handled by the resize/scroll window events (i.e. CSS changes in the layout
	* that changes the position of the anchor).
	*
	* For these situations, we refresh the popover every 0.5s
	*/
	const intervalHandle = setInterval( refreshAnchorRect, 500 );
	return () => clearInterval( intervalHandle );
	}, [] );

a9a3907 and e246157 are a bit more trivial. I think you've accounted for the former already, by the presence of anchorRect between the eslint-disable no-unused-vars of the props destructuring (it was meant to avoid the prop being assigned to the rendered element, IIRC, #15035 (comment)).

[youknowriad]

I think I updated this properly now based on your suggestions @aduth

[aduth]

For what it's worth, these hooks refactors are near impossible to review with the confidence because the changes are so dramatic, it may as well be a total rewrite. It's also not clear to me what benefits we're hoping to gain here, outside working out how hooks can serve us in developing components. In its current form, we lose a ton of useful context from named class functions and inline documentation, substituted instead with a scattered set of sparsely-documented effect callbacks. I'm not sure if the idea is that these effects could be individually pulled out into their own functions, but since that's not proposed here (and we're setting the precedent that it's not expected), it's hard to view this as an improvement.

Would it be fair to promote a pattern, where if previously:

class MyComponent extends Component {
	constructor() {
		// setup foo
	}

	componentDidUpdate() {
		// adapt foo
	}

	dealWithFoo() {}

	render() {}
}

It would roughly translate to a hooks-equivalent:

/**
 * Useful documentation explaining the purpose of this custom hook on the
 * lifecycle of a component in which it's used.
 */
function useFoo() {}

function MyComponent() {
	useFoo();

	// ...
}

[aduth]

Shouldn't need to be indented more than the previous line.

[youknowriad]

I think the main gain for me here is that the flow of the computation is made clear (and the dependencies between the different variables) which helps with the maintenability.

• anchor computed and refreshed when necessary
• popover size computed refreshed when necessary
• popover positionned and adjusted based on the two dependencies (anchor and popover size).

If you take a look at the previous implementation it's not as easy as setup, adapt... we have several functions that calls each other and it's basically impossible to follow the "flow" of things.

[youknowriad]

useFoo

I think the pattern you're proposing is a good one, the downside is that we'll end up with these hooks with several arguments and hooks that don't make sense outside the component itself which asks the question of whether we need to extract them or not.

@aduth I'll try that pattern here to see if I can make it work.

[nerrad]

Would it be fair to promote a pattern, where if previously:

I've recently started to work with hooks more in projects I'm working on and these are generally the constraints on whether I extract something to a custom hook:

• is it usable outside the component?
• Is it clear from the custom hook name and arguments what it's implementation is for?

An example of a custom hook I did recently fitting into that pattern was a useDebounce hook:

const useDebounce = ( value, delay ) => {
	const [ debouncedValue, setDebouncedValue ] = useState( value );

	useEffect( () => {
		const timer = setTimeout( () => {
			setDebouncedValue( value );
		}, delay );
		return () => clearTimeout( timer );
	}, [ value, delay ] );

	return debouncedValue;
};

[aduth]

To be clear, I don't see the primary advantage of extraction in these cases to be for reusability (though it's certainly an advantage of the pattern applied broadly), but more about documentation and readability, both in the self-documenting nature of a function name, and the actual JSDoc describing the hook function. Especially in these larger components, the refactors end up looking (to me) like a messy continuous stream of code, with no discernible boundaries or relationships. The documentation of the previous implementation wasn't great, but in the current way we're proposing to refactor these components, it seems both worsened and also not clearly set up for how we might think they should be documented and structured.

[youknowriad]

I extracted quickly those behaviors into "internal hooks". See this commit 59ba614

I'm starting to like that pattern, I think it brings clarity. Thoughts? Should I merge that here?

[aduth]

At a glance, it looks like an improvement to me. Just the added naming (useThrottledWindowScrollOrResize, etc) help provide some additional context for what to expect and, even if not added now, offers an obvious pattern for how the behaviors should be documented. Minimizing the amount of logic which takes place in the component itself makes it easier to evaluate its rendering behavior.

[youknowriad]

@aduth I agree, It does feel way better especially with the JSDocs (I added them).

[nerrad]

To be clear, I don't see the primary advantage of extraction in these cases to be for reusability (though it's certainly an advantage of the pattern applied broadly), but more about documentation and readability, both in the self-documenting nature of a function name, and the actual JSDoc describing the hook function

Good points. And I like the extraction you did @youknowriad too 👍