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

Add basic support for custom HOCs #60

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HorusGoul
Copy link

Fixes #59 for MobX's observer and other custom HOCs that follow the most common pattern.

HOC pattern covered (simple HOCs):

export default observer(Component);

HOC patterns not covered because we would need to refactor to look for component identifiers in other argument positions (not just the first), and most likely generalize the implementation for redux's connect HOC.

export default withMultipleParameters(
  () => {},
  () => {},
  Component
);
export default reduxLikeConnect(
  () => {},
  () => {},
)(Component);

Adding support for simple HOCs brings the most value because HOCs with weird implementations, such as the one with multiple parameters and the component coming last, might be rare (we have some of those in my work codebase, but we'll replace these with hooks).

As follow-up work, it might be nice to detect exported components with HOCs that aren't in the customHOCs list and raise a particular error message that tells the developer about the customHOCs setting.

Thoughts? cc @ArnaudBarre

{
name: "should be invalid when custom HOC is used without adding it to the rule configuration",
code: "const MyComponent = () => {}; export default observer(MyComponent);",
errorId: ["localComponents", "anonymousExport"],
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to add support for multiple error IDs since we don't have a dedicated message for HOCs. I'm unsure how difficult it would be to detect HOCs while differentiating them from other types of exports and create a dedicated message ID

Copy link
Owner

@ArnaudBarre ArnaudBarre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!
I dont think the redux connect pattern works though, init.callee.type with be another call expression in that case I think.

For the part custom message suggestion, I think it can make sense to have a dedicated message when:

  • At least one local component was found (keep the name of first one in a variable, or all in a list)
  • The default export is a call expression, and one of the arg is an identifier of a local component

While this will not work when the default export is first, I think it should allow the common case to work without too much false positive

Both this improvements are totally optional and can be either done in this PR, by you in another PR or later when someone request it (I've been using only hooks for 5 years so this cases are not my priority) and we can merge this as is, your call!

@HorusGoul
Copy link
Author

Let's merge it! If someone else needs this maybe we can look into adding those improvements as well 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ignore warnings when exporting a MobX observer component
2 participants