WordPressComponentProps: Only add refs when they are actually added #43610
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
mirka
commented
Aug 25, 2022
ciampo
reviewed
Aug 25, 2022
There was a problem hiding this comment.
Code changes look good and the project builds correctly, including Storybook.
I don't love that
WordPressComponentPropsnow has four parameters, but in the end it seemed easier to maintain than creating a separateWordPressComponentPropsWithoutReftype.
I'm also not in love with the 4th parameter — it's not easy to discover, and it forces devs to define the previous parameters (even if they otherwise could have been omitted in favor of their default value).
Adding a WordPressComponentPropsWithoutRef would also align better with the fact that React exposes a ComponentPropsWithRef and a ComponentPropsWithoutRef.
How much more complicated would the implementation be, if we had to go that way?
mirka
changed the title
|
Size Change: 0 B Total Size: 1.24 MB ℹ️ View Unchanged
|
|
Thanks for nudging me to try harder, I think I uncovered a critical detail that was straight-up wrong. As a result, it's much more simpler now. I've updated the PR description, and will add a few inline comments. |
mirka
commented
Aug 26, 2022
Comment on lines
+30
to
37
| // @ts-expect-error | ||
| export const View: WordPressComponent< | ||
| 'div', | ||
| ViewProps & RefAttributes< any > | ||
| > = styled.div``; | ||
|
|
||
| View.selector = '.components-view'; | ||
| View.displayName = 'View'; |
There was a problem hiding this comment.
Here we explicitly add the ref because styled.div accepts it.
(This part is kind of hacky to begin with 🙁 I might be able to untangle it if I understood what the selector property is for...)
There was a problem hiding this comment.
I might be able to untangle it
What do you have in mind?
if I understood what the
selectorproperty is for...
I believe it's used in contextConnect:
gutenberg/packages/components/src/ui/context/context-connect.ts
Lines 76 to 77 in d64dab9
Explanation:
gutenberg/packages/components/src/ui/context/wordpress-component.ts
Lines 39 to 47 in 515928d
There was a problem hiding this comment.
component interpolation in styled components
Does this refer to this kind of thing? 👇
styled.div`
${ MyComponent } {
color: black;
}
`;I might be able to untangle it
What do you have in mind?
A lot of the ts-ignores have to do with the fact that the WordPressComponent type defines the custom properties select and __contextSystemKey__ as required. Things could be easier if they were marked as optional.
Another thing is this line:
I don't see how WrappedComponent could ever have a __contextSystemKey__ property at that point. If someone ran the same component through multiple contextConnects? So either I don't fully understand what's going on, or this "merged namespaces" stuff can be cut.
In any case, these concerns are in follow-up territory.
There was a problem hiding this comment.
In any case, these concerns are in follow-up territory.
Definitely, let's discuss in a follow-up!
ciampo
reviewed
Aug 29, 2022
There was a problem hiding this comment.
I also think that things look much better!
Just a few questions left:
- Should we wait for any more progress happening in Components: Add contextConnectWithoutRef() to bypass ref forwarding #43611, before merging this PR?
- Are there any components that already accept
refas a normal prop (i.e. without being explicitly wrapped inforwardRef)? If so, is the plan for them to refactor them gradually as we convert them to TypeScript, or should we refactor them as part of this PR? - Do we need to make any changes to the contributing guidelines, especially re. handling
refs as part of the TypeScript migration ?
Comment on lines
+30
to
37
| // @ts-expect-error | ||
| export const View: WordPressComponent< | ||
| 'div', | ||
| ViewProps & RefAttributes< any > | ||
| > = styled.div``; | ||
|
|
||
| View.selector = '.components-view'; | ||
| View.displayName = 'View'; |
There was a problem hiding this comment.
I might be able to untangle it
What do you have in mind?
if I understood what the
selectorproperty is for...
I believe it's used in contextConnect:
gutenberg/packages/components/src/ui/context/context-connect.ts
Lines 76 to 77 in d64dab9
Explanation:
gutenberg/packages/components/src/ui/context/wordpress-component.ts
Lines 39 to 47 in 515928d
Nope, we're at a good point to merge 👍
Good question. The good thing about the approach in this PR is that it immediately corrects these mistakes, with no refactoring required. Sweeping the components package, I found these components to accept
From now on, a
Good point! I think we should remove the step about adding a |
ciampo
approved these changes
Aug 31, 2022
There was a problem hiding this comment.
I believe this PR needs a rebase to include this change to unblock e2e tests
I think we should remove the step about adding a forwardRef/contextConnect as part of the TS migration. They can be added when driven by an actual need, but shouldn't be part of the standard migration. Does that sound right?
Sounds good to me 🆗
Good question. The good thing about the approach in this PR is that it immediately corrects these mistakes, with no refactoring required.
That sounds great, thank you for checking! As far as no components are expecting them to accept a ref, then we're good!
🚀
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.
What?
Remove the
refprop fromWordPressComponentProps.The ad hoc
Omits on the following components have been removed:Why?
The
WordPressComponentPropstype always added arefprop, even though thepropsobject of aForwardRefRenderFunction(i.e. the function to be passed toforwardRef()) should never even contain arefprop.The
refprop should only be mixed into thepropsobject type only after the component is wrapped byforwardRef().How?
Don't include the
refinWordPressComponentProps, but instead allow it to be added fromWordPressComponentFromPropsorWordPressComponent.Testing Instructions
npm run storybook:devrefprop is still omitted from the props table.