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

[fizwidget]

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]

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

[AriPerkkio]

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]

Codecov Report

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

@@           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.

[ljharb]

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?

[ljharb]

Suggested change

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

same change throughout

[ljharb]

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]

@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.

[AriPerkkio]

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.

[AriPerkkio]

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

[fizwidget]

Thanks all for looking at this so quickly! 🙂