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

[Fix] no-unstable-components: improve handling of objects containing render functions #3111

Merged
merged 1 commit into from Oct 24, 2021

Conversation

fizwidget
Copy link
Contributor

@fizwidget fizwidget commented Oct 23, 2021

Hi folks, I ran into some false-positives when trying to enable this rule in a repository I work in. Hopefully the new test cases make the problem clear, but to summarise, this pattern currently causes the rule to fail (regardless whether allowAsProps is set):

<SomeComponent>
  {thing.match({
    loading: () => <Loading />,
    success: () => <Success />,
    failure: () => <Error />,
  })}
</SomeComponent>

I consider this to be a false-positive because there aren't actually any unstable components here - the functions are being used as "render functions", not as React components.

This PR tweaks the rule such that:

  • When allowAsProps is disabled, this pattern should only be allowed if the property names are prefixed with render.
  • When allowAsProps is enabled, this pattern should be allowed regardless of whether the property names are prefixed with render.

This effectively means object properties and component properties will be treated in the same way (which is not currently the case). Seems logical to me - let me know if I'm missing something though! 🙂

Cheers,
James

@fizwidget fizwidget changed the title [Fix] no-unstable-components: improve handling of objects containing render function props [Fix] no-unstable-components: improve handling of objects containing render functions Oct 23, 2021
@@ -500,6 +572,9 @@ ruleTester.run('no-unstable-nested-components', rule, {
return <Table rows={rows} />;
}
`,
options: [{
allowAsProps: true,
}],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand why this case was previously allowed without allowAsProps. There might be something I'm missing here..?

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, this should not have been an allowed pattern. Requiring the { allowedAsProps: true } here is correct.

It was marked as error by initial implementation. It seems there were some changes made to general component detection which affected this. 860ebea#diff-2c6ecc4f3063d33adbe4b551631a53d57725100435423ed732f4e8ea752d821aL1013

@codecov-commenter
Copy link

codecov-commenter commented Oct 23, 2021

Codecov Report

Merging #3111 (cf47696) into master (182e95d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3111   +/-   ##
=======================================
  Coverage   97.50%   97.51%           
=======================================
  Files         118      118           
  Lines        7870     7877    +7     
  Branches     2810     2814    +4     
=======================================
+ Hits         7674     7681    +7     
  Misses        196      196           
Impacted Files Coverage Δ
lib/rules/no-unstable-nested-components.js 98.27% <100.00%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 182e95d...cf47696. Read the comment docs.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

It sounds good, and I like that you've only added test cases and not removed or changed any.

When allowAsProps is enabled, then I agree that the naming of the functions shouldn't matter at all.

However, regardless, the functions created here are passed as function arguments, not directly placed in jsx. There's no way the linter can know for sure what the function being called does with them.

This means that we can risk some false positives, and warn on anything that's a CallExpression argument, or, risk some false negatives, and ignore anything that's a CallExpression argument.

It kind of seems like the latter is a better choice. What do you think?

Comment on lines +575 to +577
options: [{
allowAsProps: true,
}],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
options: [{
allowAsProps: true,
}],
options: [{ allowAsProps: true }],

same change throughout

Comment on lines +405 to +416
function ParentComponent() {
const thingElement = thing.match({
renderLoading: () => <div />,
renderSuccess: () => <div />,
renderFailure: () => <div />,
});
return (
<SomeComponent>
{thingElement}
</SomeComponent>
)
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function ParentComponent() {
const thingElement = thing.match({
renderLoading: () => <div />,
renderSuccess: () => <div />,
renderFailure: () => <div />,
});
return (
<SomeComponent>
{thingElement}
</SomeComponent>
)
}
function ParentComponent() {
const thingElement = thing.match({
renderLoading: () => <div />,
renderSuccess: () => <div />,
renderFailure: () => <div />,
});
return (
<SomeComponent>
{thingElement}
</SomeComponent>
)
}

same change throughout

@AriPerkkio
Copy link
Contributor

@ljharb I'll also take a look at these changes tomorrow/in 2-3 days. If possible let's not rush merging these.

@fizwidget thanks for the improvements! After creating this rule I've learned a ton of AST and static code analysis. I've realized I tried to accomplish too much with the initial implementation. Some thing just are not possible with static analysis. It is very likely that your changes are completely valid.

@ljharb ljharb marked this pull request as draft October 23, 2021 16:02
Copy link
Contributor

@AriPerkkio AriPerkkio left a comment

Choose a reason for hiding this comment

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

The logic looks good to me. These changes will only prevent reporting some previous false positives and will not report anything new. Sounds safe to me. Also the test cases seem to cover all cases well.

However, regardless, the functions created here are passed as function arguments, not directly placed in jsx. There's no way the linter can know for sure what the function being called does with them.

Yes, this rule is too aggressively reporting some patterns. It is making too many assumptions about component/function/prop usage which simply cannot be made due to nature of static code analysis. However this is out of scope regarding this pull request.

@@ -500,6 +572,9 @@ ruleTester.run('no-unstable-nested-components', rule, {
return <Table rows={rows} />;
}
`,
options: [{
allowAsProps: true,
}],
Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, this should not have been an allowed pattern. Requiring the { allowedAsProps: true } here is correct.

It was marked as error by initial implementation. It seems there were some changes made to general component detection which affected this. 860ebea#diff-2c6ecc4f3063d33adbe4b551631a53d57725100435423ed732f4e8ea752d821aL1013

@ljharb ljharb marked this pull request as ready for review October 24, 2021 15:55
@ljharb ljharb merged commit cf47696 into jsx-eslint:master Oct 24, 2021
@fizwidget
Copy link
Contributor Author

Thanks all for looking at this so quickly! 🙂

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

Successfully merging this pull request may close these issues.

None yet

4 participants