View: Fix TypeScript types #60919

mirka · 2024-04-19T17:43:06Z

Required for #60796

What?

Fixes type errors related to the View component that are surfaced when @types/react is updated to v18.2.79.

Why?

@types/[email protected] is a bit stricter, surfacing a problem in the View component type. Or more accurately, a problem with how we use the WordPressComponentProps utility type. It just happened to manifest at the View component, when consumer components like Navigator used it polymorphically and triggered actual prop discrepancies between div and button.

This is a minimal illustration of how we need to write our types, for a consumer-provided as prop to properly influence the accepted props:

- function Foo( { as, ...restProps }: WordPressComponentProps< {}, 'div' > ) {
+ function Foo< T extends React.ElementType = 'div' >( {
+  as,
+  ...restProps
+ }: WordPressComponentProps< {}, T > ) {
  const Component = as || 'div';
  return <Component { ...restProps } />;
}

// This errors in the first implementation, but is correct in the second.
// (`href` is not a valid prop for `div`, but valid for `a`)
<Foo as="a" href="https://example.com" />;

I believe this kind of problem could manifest in any of our polymorphic components that use WordPressComponentProps to define props without having a generic at the component-level.

How?

Reorganize the View component so it can be typed better, and add a generic to better support polymorphism.

Testing Instructions

Type checks should pass in the current package.json setup, as well as this one with updated React types:

diff --git a/package.json b/package.json
index e020a7126b..f98ca35651 100644
--- a/package.json
+++ b/package.json
@@ -137,9 +137,9 @@
 		"@types/npm-package-arg": "6.1.1",
 		"@types/prettier": "2.4.4",
 		"@types/qs": "6.9.7",
-		"@types/react": "18.0.21",
+		"@types/react": "18.2.79",
 		"@types/react-dates": "21.8.3",
-		"@types/react-dom": "18.0.6",
+		"@types/react-dom": "18.2.25",
 		"@types/requestidlecallback": "0.3.4",
 		"@types/semver": "7.3.8",
 		"@types/sprintf-js": "1.1.2",

mirka · 2024-04-22T17:12:31Z

packages/components/src/item-group/item/hook.ts

-		asProp ||
-		( ( typeof onClick !== 'undefined'
-			? 'button'
-			: 'div' ) as ElementType );


Removed unnecessary type cast.

mirka · 2024-04-22T17:27:55Z

packages/components/src/view/component.tsx

-> = styled.div``;
-
-View.selector = '.components-view';
-View.displayName = 'View';


Explicit assignment of displayName is no longer needed.

github-actions · 2024-04-22T18:42:47Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mirka <[email protected]>
Co-authored-by: DaniGuardiola <[email protected]>
Co-authored-by: tyxla <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

DaniGuardiola

This makes sense to me since there is no way around generics for actual polymorphic types. Kind of unfortunate to have to rely on "as" IMO but that's a separate discussion for another day about a pre-existing circumstance.

I do wonder if there might be a better temporary solution (even if hacky) if we're planning on addressing this soon. Adding a generic can be hard to undo.

mirka · 2024-04-23T12:53:36Z

I do wonder if there might be a better temporary solution (even if hacky) if we're planning on addressing this soon. Adding a generic can be hard to undo.

"Addressing this" as in, moving from an as prop pattern to a render prop pattern? I want to understand what kind of scenarios we could run into that would make us regret adding the generic.

Out of scope for this PR, but we should also think about what to do about View in the context of #59418. It's probably a undesirable component to proliferate, and plus it's based on Emotion.

tyxla

Looking mostly good to me 👍

I wonder if we could have avoided the markup differences, unless I'm missing a good reason to have them in the first place?

Out of scope for this PR, but we should also think about what to do about View in the context of #59418. It's probably a undesirable component to proliferate, and plus it's based on Emotion.

There's some sparse usage of the __experimentalView component across the plugin directory: https://wpdirectory.net/search/01HW5GNQWJ0EYYKZGAW9PJ0CBD so we could likely discourage it in the future without pain, especially if we head in the direction of abandoning Emotion.

tyxla · 2024-04-23T12:55:53Z

packages/components/src/view/component.tsx

+	selector: '.components-view',
+} );

 export default View;


Is there a good reason to continue exporting the default? Can't we just re-export the named export?

I think I'm ok with removing default exports, but that has kind of been a convention across the package, so it might be better to address as part of a package-wide change.

Indeed, not a blocker for the PR, but definitely something to consider at some point.

tyxla · 2024-04-23T13:02:07Z

packages/components/src/view/component.tsx

+	{ as, ...restProps }: WordPressComponentProps< {}, T >,
+	ref: React.ForwardedRef< any >
+) {
+	return <PolymorphicDiv as={ as } ref={ ref } { ...restProps } />;


Feels a bit off to have PolymorphicDiv appear in markup now, because it's an internal behavior of View and it's not immediately clear where it comes from. I wonder if we should keep the same naming externally so resulting markup doesn't change.

I think "PolymorphicDiv" is a clear label that both conveys intent and is searchable, but would you prefer something prefixed like "ViewPolymorphicDiv"?

Otherwise we need to add some overhead like const styles = { View: styled.div`` } or whatever to get around the name collision within this file. Which is not worth it IMO, especially since the class names will be completely hashed in prod anyway.

I'm just worried about the potential confusion that comes from the fact that the component is called View but it's visualized in markup as PolymorphicDiv.

In practice, this could be confusing for folks who are going to be looking for a PolymorphicDiv component, and it could potentially be an issue for usages who were dynamically targeting selectors (which they, of course, shouldn't do) based on the presence of -View- in the classname.

But if you feel this shouldn't be an issue because those internals were never official and folks should never have relied on them, then we're good.

Oh ok, it is a non-issue for the selector usage scenario, because the Emotion class names are completely hashed in prod (<div class="css-0 e19lxcc00">). They're impossible to use as selectors, even hackily.

For the dev mode debugging scenario, in my experience the human-readable Emotion class names are usually a soup of internal variable names, so it's only useful for identifying things when you already know what strings you're looking for.

Thanks for confirming. Let's 🚢 !

tyxla

LGTM, provided that we're good with the markup changes 🚢

mirka added 4 commits April 20, 2024 02:35

View: Fix TypeScript types

4d01937

ItemGroup: Remove unneeded typecasting

b19f873

Navigator: Fix View-related types

ee09e5d

Update snapshots

cc9e53c

mirka added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Apr 19, 2024

mirka self-assigned this Apr 19, 2024

mirka added 6 commits April 22, 2024 23:08

Navigator: Remove polymorphism

9ce8297

View: Widen types

c3c9e3b

Navigator: Revert changes

c128a86

Fixup

2b40db5

View: Make generic

b206063

View: Add back selector

38d02d6

mirka commented Apr 22, 2024

View reviewed changes

mirka added 2 commits April 23, 2024 02:32

Add changelog

f5391f2

Merge branch 'trunk' into fix/view-types

6486252

mirka marked this pull request as ready for review April 22, 2024 18:42

mirka requested a review from ajitbohra as a code owner April 22, 2024 18:42

mirka requested a review from a team April 22, 2024 18:43

mirka mentioned this pull request Apr 22, 2024

Upgrade @types/react package and @types/react-dom #60796

Merged

DaniGuardiola approved these changes Apr 22, 2024

View reviewed changes

tyxla reviewed Apr 23, 2024

View reviewed changes

tyxla approved these changes Apr 23, 2024

View reviewed changes

mirka merged commit b6f4947 into trunk Apr 24, 2024

mirka deleted the fix/view-types branch April 24, 2024 18:22

github-actions bot added this to the Gutenberg 18.3 milestone Apr 24, 2024

View: Fix TypeScript types #60919

View: Fix TypeScript types #60919

Uh oh!

Conversation

mirka commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What?

Why?

How?

Testing Instructions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaniGuardiola left a comment

Choose a reason for hiding this comment

Uh oh!

mirka commented Apr 23, 2024

Uh oh!

tyxla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyxla left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mirka commented Apr 19, 2024 •

edited

Loading

github-actions bot commented Apr 22, 2024 •

edited

Loading