BaseControl: Migrate to TypeScript #39468
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mirka
added
[Type] Code Quality
|
Size Change: +4 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
mirka
commented
Mar 15, 2022
| export const WithVisualLabel: ComponentStory< typeof BaseControl > = ( | ||
| props | ||
| ) => { | ||
| BaseControl.VisualLabel.displayName = 'BaseControl.VisualLabel'; |
There was a problem hiding this comment.
Interestingly, when I try to set this display name in the base-control/index.tsx file instead of here, the docgen refuses to extract types from BaseControl 🤷 I don't think we'll run into this problem a lot though. Also I guess it isn't too weird that we're setting the display name here, because it's mainly to make the Storybook code snippet understandable.
There was a problem hiding this comment.
Could this be related to the subcomponents bug that you mentioned before?
the docgen refuses to extract types from
BaseControl
What do you mean by that? I tried to comment this line out but didn't notice any changes in Storybook (although I'm probably missing something)
There was a problem hiding this comment.
Could this be related to the
subcomponentsbug that you mentioned before?
My hunch is that it's more related to the thing where we need to export the component individually as a named export (e.g. here) for the docgen to extract prop types. If you try to change a property on that exported module directly in the same file (e.g. ConnectedHeading.displayName = 'foo'), then the export suddenly stops doing its magic.
🙂 Docgen works as expected
// component.tsx
export const MyComponent = /* ... */;
// consumer.tsx
import { MyComponent } from "./component";
MyComponent.displayName = "Hello";☠️ Docgen ignores my prop types
// component.tsx
export const MyComponent = /* ... */;
MyComponent.displayName = "Hello";I'm not familiar enough with the internals to say why this happens, but to someone who is familiar, I bet the reason is quite obvious!
mirka
commented
Mar 15, 2022
mirka
force-pushed
the
refactor/ts-base-control
branch
from
f1eade4
to
181d3a9
Compare
mirka
commented
Mar 18, 2022
ciampo
reviewed
Mar 21, 2022
There was a problem hiding this comment.
Thank you for working on this, @mirka !
I left some comments, but my main piece of feedback is if you have you considered using the WordPressComponentProps type, instead of FunctionComponent ? We tend to use this type for the majority of our TypeScript components.
The new Storybook examples are great!
ciampo
reviewed
Mar 21, 2022
If I understand correctly, this type shouldn't be used if the component isn't forwarding its rest props to the given element type. So like |
|
Thanks for the review! All points have been addressed. |
ciampo
approved these changes
Mar 23, 2022
There was a problem hiding this comment.
LGTM 🚀 Feel free to merge once the remaining comments are addressed.
Hopefully we'll be soon able to convert the unit tests to TypeScript too (#39436)!
will imply that all div props are allowed, for example style. And that would be incorrect in this case.
This is indeed a fair point. We can look into migrating to WordPressComponentProps at a later stage if/when needed.
| export const WithVisualLabel: ComponentStory< typeof BaseControl > = ( | ||
| props | ||
| ) => { | ||
| BaseControl.VisualLabel.displayName = 'BaseControl.VisualLabel'; |
There was a problem hiding this comment.
Could this be related to the subcomponents bug that you mentioned before?
the docgen refuses to extract types from
BaseControl
What do you mean by that? I tried to comment this line out but didn't notice any changes in Storybook (although I'm probably missing something)
|
Probably related: styleguidist/react-docgen-typescript#339 Noting that I hit an interesting issue where the For context: When the
|
This matches the readme and common-sense usage of this component
This reverts commit 6e59dbe480f162683504cdb3c4fa691e6e43d0d6. # Conflicts: # packages/components/src/base-control/types.ts
Without this, the docgen is confused and fails to mark `children` as required
mirka
force-pushed
the
refactor/ts-base-control
branch
from
e90416d
to
e6fa3dd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #35744
What?
Converts the
BaseControlcomponent to TypeScript.Why?
Improves devex and code quality through type safety, IDE type hints, and better Storybook documentation.
How?
Testing Instructions
npm run storybook:devand check the Docs tab forBaseControl. It should be useful and understandable.For easier code review, exclude the commit where I just changed the file extensions.
✅ Tests pass