-
-
Notifications
You must be signed in to change notification settings - Fork 296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(defaultPropsHandler): Fully support forwardRef #350
feat(defaultPropsHandler): Fully support forwardRef #350
Conversation
/** | ||
* function, lazy, memo, forwardRef etc components can resolve default props as well | ||
*/ | ||
if (!isReactComponentClass(componentDefinition)) { | ||
statelessProps = getStatelessPropsPath(componentDefinition); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to change that too. This was always a bit misleading and with hooks it is definitely wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you referring here? Changing the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming yes. Doesn't matter really since it's internals only. This should probably be handled in a different PR and applied consistently. But I don't know if this would change some react-docgen/utils API so I guess this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you rename it to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. Probably just getFunctionPropsPath or just getPropsPath. Naming is hard but I don't think this discussion serves any purpose.
Thanks for your contribution again. Looks good to me, I just wonder can you confirm it also works when the Component is not directly nested? Like: import React from 'react';
const Component = ({ foo = 'bar' }, ref) => (<div ref={ref}>{foo}</div>);
React.forwardRef(Component); By looking at the code we might have to do another |
I'm not sure we should support this pattern since the argument for forwardRef isn't a component but a render function that takes a second argument. This may be technically only but you couldn't use const Component = (props, ref) => {}
Component.propTypes = {}; // Warning: forwardRef render functions do not support propTypes or defaultProps. Did you accidentally pass a React component?
export default React.forwardRef(Component); If you think this is too rigid I can add an additional test and see if it works. |
I see that makes sense. 👍 |
* test(failing): defaultPropsHandler and forwardRef * feat(defaultPropsHandler): support forwardRef # Conflicts: # src/handlers/__tests__/__snapshots__/defaultPropsHandler-test.js.snap
In
React.forwardRef(({ foo = 'bar' }, ref) => <div ref={ref}>{foo}</div>)
the default props handler would not detect thatfoo
had a default value.I was curious if #343 would've fixed this as well but this is not the case.