feat(react): allow proptype displayname match in childOf checks (WIP) #2785
Conversation
Allow displayname match to count for proptype checks in: prop-types/childrenOf prop-types/childrenOfType The impetus for this was react-hot-loader proptype checks failing. The hot loader wraps classes in a proxy wrapper so the strict class match doesn't work. https://github.com/gaearon/react-hot-loader#checking-element-types
|
Deploy preview for the-carbon-components ready! Built with commit 1033645 https://deploy-preview-2785--the-carbon-components.netlify.com |
|
Deploy preview for carbon-components-react ready! Built with commit 1033645 https://deploy-preview-2785--carbon-components-react.netlify.com |
finken2
changed the title
|
@asudoh Thanks for taking a look and providing feedback. I see what you're saying about displayName, but that mostly looks to be a problem in production. The validator won't even be running there, so it shouldn't be an issue. The displayName seems to stay consistent in development mode, at least for the components i looked at. i've looked at some of the other attempts including: I tried the createElement approach a little this morning, and while it works for comparison, it leads to the problem of warnings for missing required properties. I like the displayName approach since it works for the simple development case currently, has minimal impact, and shouldn't make things any worse. Let me know what you think. Another alternative could be just making the proptypes in the ListBox components more general (PropTypes.object). I can continue down any of these paths if we want to. Let me know. |
|
Thank you for exploring some of the different options - The concern around |
| @@ -21,14 +22,20 @@ const childrenOf = expectedChildTypes => { | |||
| const expectedDisplayNames = expectedChildTypes | |||
| .map(child => getDisplayName(child.type || child)) | |||
| .join(', '); | |||
| const expectedChildElementTypes = expectedChildTypes.map( | |||
| child => React.createElement(child).type | |||
There was a problem hiding this comment.
We often go down this road but get stumped when props are marked as required for a child and this call will trigger warnings as a result 😞 Does this PR offer a way around that, or is this not a problem anymore?
There was a problem hiding this comment.
nope, still a problem as of now. See my previous comment. and @asudoh 's reply.
I'll try out createElement a bit more as time allows. The thing with displayName, we're not really depending on it in a way that would break. So worst case, the check doesn't work, and you're right back where you started, seeing a react warning in dev console. But best case, checking displayname as an additional matching possibility gets rid of those warnings. |
|
Hi @finken2! 👋 Wanted to check back in on the status of this PR. Do you think you'll have any time to continue your explorations, or should we close this PR in the meantime? Please note that closing a PR doesn't mean that we don't want to accept it! Just means that it'll be revisited when you get the chance 😄 |
|
@joshblack hey josh, i've been meaning to ping you guys on Slack to continue the discussion. I'd really still like to get this in with the displayName check. I think that adds some value, and shouldn't hurt anything. |
|
Hi @finken2 if you can explore an alternate approach it'll be great. The concern around |
|
Deploy preview for carbon-elements ready! Built with commit 1033645 |
|
@asudoh I wasn't able to find any solution for the required props issue mentioned previously when going the component creation route. I'll close this PR for now and hopefully get back to it at some point if/when things change. |
Allow displayname match for proptype checks in:
prop-types/childrenOf
prop-types/childrenOfType
The impetus for this was react-hot-loader proptype checks failing. The react-hot-loader wraps classes in a proxy wrapper so the strict class match doesn't work:
https://github.com/gaearon/react-hot-loader#checking-element-types
gaearon/react-hot-loader#304
This causes a react warning in the chrome dev console during development when using the carbon-react dropdown components.
It seems reasonable to allow displayname to count for these checks. Currently, it appears that these childOf functions are only used in the ListBox set of components.