Ts blocks #5668
Ts blocks #5668
Conversation
alexflorisca
added
type: refactor
rubikuserbot
requested review from
a team and
mikejolley
and removed request for
a team
Remove this once getCollection selector and resolver is c...Remove this once getCollection selector and resolver is converted to TS. https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/e0531fbb9f503a50e1b1b0ab6f4d09bc859252c7/assets/js/base/context/hooks/use-store-products.ts#L47-L51🚀 This comment was generated by the automations bot based on a
|
|
Size Change: +507 B (0%) Total Size: 812 kB
ℹ️ View Unchanged
|
| @@ -34,8 +44,8 @@ export const useStoreProducts = ( query ) => { | |||
| query, | |||
| } ); | |||
| return { | |||
| products, | |||
| totalProducts: parseInt( totalProducts, 10 ), | |||
| products: products as ProductResponseItem[], // TODO: Remove this once getCollection selector and resolver is converted to TS. | |||
There was a problem hiding this comment.
On line 38 above we call the useCollection hook, which looks up some data from a store and returns it. We need to convert quite a few other things to TS in order to get the correct types here (useCollection hook, some selectors and resolvers in the data folder, and it's not an easy task as most of these return generic nested objects so it needs a little more thought. I didn't have the time to convert everything needed by the useStoreProducts hook in order to return the correct types from here, so I cast it to an array of ProductResponseItem which we are expecting here until we get to convert all other relevant hooks and state related code to TS.
| @@ -54,7 +55,6 @@ const ProductSortSelect = ( { onChange, readOnly, value } ) => { | |||
| ), | |||
| }, | |||
| ] } | |||
| readOnly={ readOnly } | |||
There was a problem hiding this comment.
removed the readOnly property from the SortSelect component as it was never used, and was being applied to a HTML Element which is not valid
| return ( | ||
| <SortSelect | ||
| className="wc-block-product-sort-select wc-block-components-product-sort-select" | ||
| name="orderby" |
There was a problem hiding this comment.
removed invalid name property being passed to SortSelect component. This is not defined or used in SortSelect
|
Hi Alex, got a question regarding typing the components. Is there a reason we were not using generic Ie. for interface FilterElementLabelProps {
name: string;
count: number;
}
const FilterElementLabel: React.VFC< FilterElementLabelProps > = ( {
name,
count,
} ) => {and for interface FormProps {
className: string;
onSubmit: ( event: FormEvent ) => void;
}
const Form: React.FC< FormProps > = ( {
className,
children,
onSubmit = ( event ) => void event,
} ) => {
// ...
}Using I've noticed it was used only once in the Chip component but I believe this to be the most explicit way to type functional components in React and would love us to follow this typing pattern. What do you think? |
|
Hi @tomasztunik, Thanks for your comment. I agree that using |
| export const getBlockMap = ( blockName ) => | ||
| export const getBlockMap = ( | ||
| blockName: string | ||
| ): Record< string, React.ComponentType > => |
There was a problem hiding this comment.
I see some types in our checkout block registry which look related and use:
component:
| LazyExoticComponent< React.ComponentType< unknown > >
| ( () => JSX.Element | null )
| null;
Is this typing the same thing? https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/8475bb8c47499ad535aceb4391d9bafa30ec3228/packages/checkout/blocks-registry/types.ts#L36
There was a problem hiding this comment.
Yep, thanks for pointing this out. I've extracted a type RegisteredBlockComponent and re-used it instead
| return ( | ||
| <> | ||
| { name } | ||
| { Number.isFinite( count ) && ( | ||
| <Label | ||
| label={ count } | ||
| label={ count.toString() } |
There was a problem hiding this comment.
I know this fixes the type, but I see some usage of this component like:
count={ blockAttributes.showCounts ? count : null }
I think this may error? We should fix the cases which consume this component, or make it optional.
There was a problem hiding this comment.
I see what you mean. Those occurrences should also pass count.toString() but they're not typed yet. The Label component expects a string as the count, but it doesn't do any number logic with the count, it just outputs it as inside a HTML element, so should be safe.
| @@ -11,14 +10,21 @@ import classNames from 'classnames'; | |||
| import './style.scss'; | |||
| import Spinner from '../spinner'; | |||
|
|
|||
| interface LoadingMaskProps { | |||
| children?: React.ReactChildren; | |||
There was a problem hiding this comment.
I see in other places in the codebase we are using ReactNode or ReactNode[] rather than React.ReactChildren; . Do you know which is best practice, and can we decide on a standard to use throughout?
There was a problem hiding this comment.
You're right, there are multiple ways to type children and this article sums them up pretty well. I suggest we stick with ReactNode or ReactNode[] for now as our codebase is so complex, that narrowing children down further will require changes at multiple levels where components are nested within each other. It will be quite time consuming to trace down every component that the children pass through and modify the types, I don't think it's worth the effort. It's something to bare in mind going forward though: being more specific with the types of children that a component is allowed is the better way to go and will catch more errors
assets/js/base/components/product-list/product-list-item/utils.tsx
Outdated
Show resolved
Hide resolved
| @@ -195,7 +207,7 @@ const ProductList = ( { | |||
| ! Number.isFinite( totalProducts ) && | |||
| Number.isFinite( previousQueryTotals?.totalProducts ) && | |||
| isEqual( totalQuery, previousQueryTotals?.totalQuery ) | |||
| ? Math.ceil( previousQueryTotals.totalProducts / perPage ) | |||
| ? Math.ceil( ( previousQueryTotals?.totalProducts || 0 ) / perPage ) | |||
There was a problem hiding this comment.
Can we force this shape earlier to avoid checking for existance of totalProducts? Just for readability sake.
There was a problem hiding this comment.
The problem here is that previousQueryTotals could be undefined. I think this is the cleanest way using the optional chaining (?.) operator and defining a default value inline if the prop isn't defined. The alternative I see would be to wrap that entire block in an if(previousQueryTotals) {} statement. I don't think we can force previousQueryTotals to never be undefined. If you have any other ideas I'm open to suggestions
|
Thanks for the review @mikejolley, I've addressed most of what you rightly pointed out and answered some of your questions inline. I've also fixed the unit tests so fingers crossed everything should be green. Let me know if you spot anything else! |
sunyatasattva
added
the
skip-changelog
This PR converts the following components to Typescript
base/components/ProductListbase/components/FilterElementLabelbase/components/FilterSubmitButtonbase/components/Formbase/components/LoadMoreButtonbase/components/LoadingMaskbase/components/Paginationbase/components/SortSelectThe ProductList component was quite tricky to get completely error free for a couple of reasons:
withScrollToTopso that it acceptsProductListas a component. The HOC expects a constructor, yetProductListreturns aJSX.Element. If anyone has any bright ideas here, I'd love to hear them. I've tried to type theOriginalComponentprop in thewithScrollToTopHOC asReact.ComponentType, as suggested hereunknown[]asProductResponseItem[]as a temporary shortcut. I've left an inline comment with more explanation about this.Testing
Automated Tests
Manual Testing
How to test the changes in this Pull Request:
User Facing Testing
These are steps for user testing (where "user" is someone interacting with this change that is not editing any code).