Create withProduct HOC

[Aljullu]

This PR includes several changes:

• Moves getProduct() to a HOC following @nerrad's suggestion for the <ProductsControl> component. This way, we will be able to reuse getProduct() in Reviews by Product without duplicating the logic.
• Converts <FeaturedProduct> to a functional component.
• Removes the retrying state from ApiErrorPlaceholder and make it rely on a isLoading prop, this way we don't have to maintain the same state in two different components.
• Moves the error messages to ApiErrorPlaceholder.

How to test the changes in this Pull Request:

To test, basically add a Featured Product block and verify it still works as expected.

[Bonus points] Test that errors are still displayed:

1. Replace this line with:

-return true;
+return new \WP_Error( 'DERP', 'You cannot read products.' );

2. Add a Featured Product block.
3. Notice the error appears as expected.

[nerrad]

Hey @Aljullu this is a great first attempt!

• there's some js unit test errors that will need looked into.
• There's still some coupling here. Imo withGetProduct should simply be withProduct (or maybe withDebouncedProduct to describe that it is debounced and essentially contains ALL the logic for the retrieval of the product. Then essentially the existing FeaturedProduct block could be a functional component because all it would contain is the contents of render (getInspectorControls could be a function internal to the component or external receiving arguments (internal probably would be fine).

[Aljullu]

@nerrad thanks for taking a look!

there's some js unit test errors that will need looked into.

Right, they were fixed in #781. I rebased and they should be passing again.

There's still some coupling here. Imo withGetProduct should simply be withProduct (or maybe withDebouncedProduct to describe that it is debounced and essentially contains ALL the logic for the retrieval of the product. Then essentially the existing FeaturedProduct block could be a functional component because all it would contain is the contents of render (getInspectorControls could be a function internal to the component or external receiving arguments (internal probably would be fine).

Agree, my initial thought was to keep componentDidMount() and componentDidUpdate() out of the HOC so other components needing getProduct() but not wanting to automatically load it on mount could also use that HOC. But after thinking it twice, I don't think there is any use-case for that in the near future, so moving them to the HOC sounds good.

I did that and converted <FeaturedProduct> to a functional component as you said. At some point I would like to split that file into smaller chunks since it's a bit too long for my taste, but that can probably been done in a follow-up so this PR doesn't get too big.

internal probably would be fine

I'm curious about this, I probably would have done them external. Is there any pros and cons of each approach?

[nerrad]

👍 looking good. I still have a few comments for feedback. As well, I noticed you've removed the debouncedGetProduct logic. Was that intentional? I was expecting to see it in the new withProduct hoc, or is that something that you felt was no longer necessary?

Also, now that data logic has been separated from presentation, what are your thoughts on adding tests for the withProduct hoc?

I'm curious about this, I probably would have done them external. Is there any pros and cons of each approach?

Pros:

• within scope of the component so you don't have to pass the arguments around
• it's clear the functions are specific to the component.

Cons:

• be wary of passing along the function as a callback on a child element as a prop. The function is re-created every time the main (parent) component is rendered so that can cause unnecessary re-renders for children. So in that case you definitely DO want to have it as an external function (or implemented with useCallback or useMemo - depending on the use-case if using React hooks).

Another consideration on whether it should be internal or external I think would be on whether there's potential for exposing the functions (especially if they are more like functional components themselves! for other consuming code).

[nerrad]

I wonder if we should put hocs in a top-level (living at the same level as components) folder? In the pull I'm working on I have js/hocs/with-searched-products.js. I don't think we should initially expose hocs publicly yet, but it does provide the option down the road for exposing them on their own package (and makes it easier to locate them too).

[Aljullu]

Absolutely. Done in 72abc25.

In a follow-up, we will need to move with-component-id.js too.

[Aljullu]

In a follow-up, we will need to move with-component-id.js too.

Done in #797.

[nerrad]

Regarding getProduct I don't think the presentation component should receive any getter but instead only the values it uses for rendering. I realize that it's being used as the onRetry method for ApiErrorPlaceholder but this to me smells like ApiErrorPlaceholder should be wrapped with the withGetProduct Hoc and exposed as ProductApiErrorPlaceholder which can then be used here. That would remove the data coupling here.

Alternatively, in future refactors we could look at having error placeholders passed in as render props which would give more flexibility to code consuming the presentation components for how errors are handled.

[Aljullu]

I have been investigating around this idea but I'm not sure to understand it yet. What would be the benefit of wrapping ApiErrorPlaceholder with withProduct if the parent component is already wrapped by that HOC? That means we are re-rendering the error component at least once just to get the props which were already loaded in its parent.

Am I missing something? I created a WIP here:

https://github.com/woocommerce/woocommerce-gutenberg-products-block/compare/fix/reset-loaded-state-in-featured-blocks...fix/reset-loaded-state-in-featured-blocks-error?expand=1

[Aljullu]

Thanks for the review again @nerrad.

+1 looking good. I still have a few comments for feedback. As well, I noticed you've removed the debouncedGetProduct logic. Was that intentional? I was expecting to see it in the new withProduct hoc, or is that something that you felt was no longer necessary?

getProduct was only called from componentDidMount() and componentDidUpdate() when the productId attribute was different. I think in both cases there is no point in debouncing them: a component shouldn't mount/unmount many times and the productId attribute shouldn't change often either.

So I didn't see any use-case for debouncing the function.

Also, now that data logic has been separated from presentation, what are your thoughts on adding tests for the withProduct hoc?

Done in cc43716. 👍

[nerrad]

I like the changes. Awesome!

Regarding the ProductApiPlaceholder. I think it's a bit more clearer to me how it would work now that you extracted getProduct( id: number ) in 4aebc34.

So what I envision now is that you'd have something like:

import { ApiErrorPlaceHolder } from './components';
import { getProduct } from './utils';

export const ProductApiErrorPlaceHolder = ( {
	className,
	error,
	isLoading,
	productId,
} ) => {
	return error ?
		<ApiErrorPlaceHolder
			className={ className }
			error={ error }
			isLoading={ isLoading }
			onRetry={ () => getProduct( productId ) }
		/> :
		null;
};

That way you don't have to pass the getProduct (essentially the new loadProduct in the hoc) through to the wrapped component. That makes the HOC a little bit more guarded and you're not exposing the data retrieval logic (which makes it more difficult to eventually export that publicly).

I think it's important to keep any data retrieval encapsulated as much as possible in the components responsible for it. This way you can also keep ApiErrorPlaceholder as an internal only component but we could eventually expose the specific ApiErrorPlaceholders for other components publicly for use (such as ProductApiErrorPlaceholder because for example in this case, we can change the internals for how a product is retrieved onRetry, but consuming code doesn't have to care about it).

[nerrad]

Ugh 🤦‍♂ ... I just realized the severe flaw with the comments I made about the ApiErrorPlaceholder in that the whole purpose for passing through the onRetry callback is so that the product retrieved on the retry is updated in the main component containing the placeholder... so, for now let's just leave it as you have it in this branch. I'm still not sure I like how the data retrieval method is coupled here but it's something we can iterate on. I'll think on it a bit more.

[Aljullu]

I implemented your suggestions @nerrad, this PR is ready for another review.

[nerrad]

Looks great!

Note, I haven't tested the existing functionality of the block as per the pull notes, but code wise this looks good to go.