[core] Handle refType in the proptypes generation
#38917
Conversation
Netlify deploy previewhttps://deploy-preview-38917--material-ui.netlify.app/ Bundle size report |
flaviendelangle
force-pushed
the
refType
branch
2 times, most recently
from
f9b8721 to
fd79bd4
Compare
| const usedCustomValidator = | ||
| previous !== undefined && | ||
| !previous.startsWith('PropTypes') && | ||
| !previous.startsWith('refType'); |
There was a problem hiding this comment.
I would prefer to move this logic inside TTP to share it with X.
For now, if we forget to copy paste on X, we have diverging before for no reason.
flaviendelangle
changed the title
refType in the proptypes generation
flaviendelangle
force-pushed
the
refType
branch
from
fd79bd4 to
1a292d8
Compare
| "description": "func<br>| { current?: { focusVisible: func } }" | ||
| } | ||
| }, | ||
| "action": { "type": { "name": "custom", "description": "ref" } }, |
There was a problem hiding this comment.
@oliviertassinari for these custom refs, it might be debatable if the new generation is actually better.
I'd say that func | { current?: { focusVisible: func } } was not very readable anyway, but still we are loosing information.
I can avoid using refType when the ref value is not an HTML element.
There was a problem hiding this comment.
Of change refType to have refDOMNodeType for the HTML elements and refType(...) that takes a param for the custom refs
actions: refType({
focusVisible: PropTypes.func,
})
There was a problem hiding this comment.
Agree, I think a ref implies access to a DOM node, which isn't the case here.
https://mui.com/base-ui/react-tabs/components-api/#Tab-prop-action feels better than: https://deploy-preview-38917--material-ui.netlify.app/base-ui/react-tabs/components-api/#Tab-prop-action.
It feels like we are hitting a limit of TypeScript → prop-types → docs. It should really be:
- TypeScript → prop-types
- TypeScript → docs
This feels better than these two: https://deploy-preview-38917--material-ui.netlify.app/base-ui/react-tabs/hooks-api/#prop-rootRef
There was a problem hiding this comment.
It feels like we are hitting a limit of TypeScript → prop-types → docs. It should really be:
That's the main goal of my architectural changes.
I created a layer that only analyses the AST trees and that should be usable by both and let both add the specificity they want.
| @@ -97,14 +98,7 @@ Button.propTypes /* remove-proptypes */ = { | |||
| /** | |||
| * A ref for imperative actions. It currently only supports `focusVisible()` action. | |||
| */ | |||
| action: PropTypes.oneOfType([ | |||
There was a problem hiding this comment.
Off-topic
Maybe for API consistency?
| action: PropTypes.oneOfType([ | |
| actionRef: PropTypes.oneOfType([ |
There was a problem hiding this comment.
I don't think it's an improvement - we lost information about the shape of the ref here. The refType should apply only to refs to HTML elements IMO.
As a side note, if I remember correctly, we're using action props to support focusVisible action, which I'd like to drop around v6 (browser support should be sufficient then).
In other cases (like menus, select, etc.) we expose the dispatch prop that accepts a function instead.
There was a problem hiding this comment.
I think the right discussion is here
I think we can close the PR
I did a proposal that could be better than the current state, but I don't think the effort is worth it for now
github-actions
bot
added
the
PR: out-of-date
|
Closing per #38917 (comment) |
Solves mui/mui-x#8326 (comment)