Skip to content
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

Detect components wrapped in HOC with all resolver #344

Merged
merged 1 commit into from Oct 25, 2019

Conversation

danez
Copy link
Collaborator

@danez danez commented Apr 13, 2019

Fixes #331

This is I think breaking as the following example will now detect a component:

myFunctionCallWhichIsNotAHOC(something, node => (<div>{node}</div>), other);

And I don't see a way to workaround this as we cannot detect if a call is a HOC or not.

@fkling
Copy link
Member

fkling commented Apr 15, 2019

This should be possible to resolve without breaking changes. I think I have some local changes that rewrote isReactForwardRefCall to a more general isReactHOC call that also handles React.memo. Maybe we need to consolidate the different resolvers better.

However, I also think relaxing the rules around HOCs would be useful, but that should probably be in resolveHOC.

@danez
Copy link
Collaborator Author

danez commented Apr 15, 2019

React.forwardRef though is not a HOC as far as I could tell. I did some simple tests in codesandbox (https://codesandbox.io/s/3v7pj8pwvp):

So for HOC this works:

Component = ...,

Component.propTypes = {};

export default HOC(Component);

This though does not work for React.forwardRef.

There the static properties have to be assigned after the call, which is not very common for HOC:

Component = React.forwardRef(...),

Component.propTypes = {};

export default Component;

It even throws when doing the first example:

Warning: forwardRef render functions do not support propTypes or defaultProps. Did you accidentally pass a React component?

I guess we could ignore that fact and treat it as HOC for simplicity which would support the valid + the invalid case for React.forwardRef?

But the breaking change here would still be needed afaics, because if a HOC receives more than one argument we need to find out which one is the Component.
Right now it is the last argument, I changed it to be the first (unless the first is obviously not a component). But in cases like HOC(ListElement, options, mapStatetoSomething) it is not really clear, unless we go through all arguments and check if it resolves to a class or stateless component.

I just realized that my comment was referring the other breaking change :D
I'm interested to see how you solved this.

@danez danez merged commit aa54200 into master Oct 25, 2019
@danez danez deleted the fix-hoc-in-all-resolver branch October 25, 2019 19:52
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential issue with findAllComponentDefinitions
2 participants