Components: Update polymorphism types to allow all attributes when specifying as - #80705
Conversation
|
Size Change: -7 B (0%) Total Size: 7.76 MB 📦 View Changed
|
| /** | ||
| * The content to render inside the wrapper. | ||
| */ | ||
| children?: ReactNode; |
There was a problem hiding this comment.
From what I understand, this should have always been here, and the previous polymorphism types incorrectly typed children for non-polymorphic components. Generally it should be expected that components define their own children types, which we're more consistent about in @wordpress/ui (e.g. ComponentProps explicitly omits children)
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Oof, type-checking failures in CI. Guessing some local cache might have misled me locally. I'll throw this back to draft temporarily and revisit it on Monday, unless someone wants to go ahead and carry the torch on this one. Edit: Nevermind, decided to tackle it now. |
| path={ `/${ tab.name }` } | ||
| // @ts-expect-error: Navigator.Button is currently typed in a way that prevents Item from being passed in | ||
| as={ Item } | ||
| isAction |
There was a problem hiding this comment.
This is safe:
gutenberg/packages/components/CHANGELOG.md
Line 3309 in c6c8e71
Navigator.Button provides onClick:
There was a problem hiding this comment.
The reason the removal has to happen is because the TypeScript error on the line above assigning as={ Item } no longer errors, and the suppression through @ts-expect-error was subsequently not checking this prop, which had been removed from the types in #35152. Once the props were checked, it flagged the prop as not part of the Item props types.
|
Flaky tests detected in 7e5d36a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/30124455510
|
|
Wow, nice work @aduth. This will likely greatly improve the performance of extenders of |
manzoorwanijk
left a comment
There was a problem hiding this comment.
Thank you for improving the performance here. I have some inline questions/suggestions.
Also, I was wondering whether we should have some type tests for the usages.
| * Compatible with Emotion/`styled` `as` props and React's `ElementType`, without | ||
| * using the default `React.ElementType` mapped type helper directly. | ||
| */ | ||
| type PolymorphicAs = |
There was a problem hiding this comment.
Should this be named something that explains what it actually is rather than what it's used for? Like PolymorphicElement, PolymorphicElementType etc.
There was a problem hiding this comment.
Sure, that does seem better 👍
Also worth noting that this separate type should ideally be temporary, as there's a lot of active work underway to eliminate Emotion altogether (#66806), at which point we won't need this.
There was a problem hiding this comment.
at which point we won't need this
I think we do? Unless we get rid of the as prop back compat. (We're removing Emotion, but keeping the as prop for back compat.)
There was a problem hiding this comment.
I dug into this a bit more. I understood that we would continue supporting as, but it was my impression from rounds of iteration with my AI agent that there was something specific about Emotion's typings that made it necessary to type it this way specifically. But on closer examination, I think my agent made an inappropriate leap based on trying to simplify this typing to something like as?: string | React.JSXElementConstructor< any > as the ideal end-case, and the only thing holding us up was Emotion's stricter treatment of keyof React.JSX.IntrinsicElements vs. string. I think this isn't so much an "Emotion" thing as it is just better more accurate types. It's a little bit of a trade-off because the simplicity of string could have better performance, and I'd be curious to test the impact once we do get rid of Emotion to see if it's worth it, but keyof React.JSX.IntrinsicElements means that we still validate that as="label" is a valid tag name.
There was a problem hiding this comment.
This is improved in 85789d2 . I opted to just inline the type to solve both problems at once. Details in the extended commit description:
Improves a couple things:
- Avoids a named type that references what it's used for (the "as" prop) rather than what it is
- Avoids comments specifically referencing Emotion as the reason it's typed the way it is, as this is an objectively better, more accurate typing
| /** | ||
| * Size of the underlying Modal. See `Modal`'s `size` prop. | ||
| */ | ||
| size?: ModalProps[ 'size' ]; |
There was a problem hiding this comment.
This prop didn't land in the README for the component.
There was a problem hiding this comment.
The short answer is that the component lacks a docs-manifest.json which would opt it into automatic documentation, and so its README is maintained manually. We could update it manually, but I'm also curious to dig into why it doesn't have a manifest 🙂
There was a problem hiding this comment.
I tried adding the docs-manifest.json and it works (also brings other missing props like isBusy), but it causes a lot of unrelated changes to the README.md (changes formatting quite a bit) that I'd suggest we tackle separately. In the interim, da70077 adds the manual prop documentation.
There was a problem hiding this comment.
Separate pull request for the auto-documentation: #80866
There was a problem hiding this comment.
We could update it manually, but I'm also curious to dig into why it doesn't have a manifest 🙂
I set up the readme autogen system pretty late in the game, so a lot of them simply aren't migrated yet. Nothing more 😄
Improves a couple things: - Avoids a named type that references what it's used for (the "as" prop) rather than what it is - Avoids comments specifically referencing Emotion as the reason it's typed the way it is, as this is an objectively better, more accurate typing
See: #80705 (comment) Since this can cascade to inner types like event handler props, we can do better than the loose `any` type as we know this will be an element.
7e5d36a to
da70077
Compare
manzoorwanijk
left a comment
There was a problem hiding this comment.
This looks good to me now. Thank you for working on this.
|
As a more visible data point, this looks to bring the typical runtime of "Type checking" in CI from about 2m40s to 1m45s (-35%). Five examples before, from the latest 5 commits on
Two examples after, from the latest commit here and the one before my last force-push: |
…ecifying `as` (#80705) * Components: Update polymorphism types to allow all attributes for all components * Components: Restore polymorphism for components passed as `as` * Sync components documentation * Update CHANGELOGs * Components: Inline PolymorphicAs typings Improves a couple things: - Avoids a named type that references what it's used for (the "as" prop) rather than what it is - Avoids comments specifically referencing Emotion as the reason it's typed the way it is, as this is an objectively better, more accurate typing * Components: Narrow generic type on polymorphic intrinsic types See: WordPress/gutenberg#80705 (comment) Since this can cascade to inner types like event handler props, we can do better than the loose `any` type as we know this will be an element. * Components: Document size prop Co-authored-by: aduth <aduth@git.wordpress.org> Co-authored-by: manzoorwanijk <manzoorwanijk@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Source: WordPress/gutenberg@b7376ac
What?
Updates the polymorphism types used in
@wordpress/componentsto expose all possible HTML attributes whenasis specified, rather than narrowing the types based on the tagName provided toas.The performance impact on TypeScript builds is significant, reducing memory usage by -47% (7.4GB to 4.0GB), TypeScript build time by -49% (13.7s to 7.0s), and internal instantiations by -54% (42.5M to 19.5M).
Why?
How?
As described in #80655 , these polymorphism types are a major contributor to overall TypeScript build times and memory consumption. While #80655 tried to fully maintain the original intent of the polymorphism types, the changes here propose a more drastic change: If
asis provided, all valid HTML attributes are accessible on that component. This means someone could do something like<VisuallyHidden as="div" htmlFor="id" />and TypeScript would consider it valid, which was not the case before now, ashtmlForis not a valid attribute fordivelements.This is meant to be a reasonable compromise given the circumstances, and how new development in
@wordpress/uileans onrenderprops instead ofaspolymorphism.renderprops don't suffer from this problem because props which are relevant to the component passed torenderare applied to the element directly.This isn't all bad though, since:
asis provided, and otherwise the component's own props are the only ones available.htmlForis astring, for example.Testing Instructions
Verify type-checking passes:
For bonus points, try some of the scenarios described under "How", like how you can't assign
htmlForunless specifyingas, or thathtmlForcan be used withas="div", or howhtmlForcan't be assigned to a type other than string.Also check performance for yourself using snippets from #80364 "Testing Instructions":
Use of AI Tools
Used Cursor IDE + Auto (likely Composer) model to research and implement, with manual iterations, particularly on the code comments.