feat(rome_js_analyze): noUselessFragments

[ematipico]

Summary

It implements #3254

The old rule had also a suggested fix, which I would like to implement in another PR. Having this PR with the correct diagnostics, would help me later to avoid regressions.

It seems now that there are some recurrent patterns emerging, I will refactor them in another PR.

Test Plan

I added new test cases

[netlify]

✅ Deploy Preview for rometools canceled.

Name	Link
🔨 Latest commit	a653bcd
🔍 Latest deploy log	https://app.netlify.com/sites/rometools/deploys/632b2a8f9357cd000928e7b2

[github-actions]

Playground for commit 818737b

[github-actions]

Parser conformance results on ubuntu-latest

js/262

Test result	main count	This PR count	Difference
Total	45879	45879	0
Passed	44939	44939	0
Failed	940	940	0
Panics	0	0	0
Coverage	97.95%	97.95%	0.00%

jsx/babel

Test result	main count	This PR count	Difference
Total	39	39	0
Passed	36	36	0
Failed	3	3	0
Panics	0	0	0
Coverage	92.31%	92.31%	0.00%

symbols/microsoft

Test result	main count	This PR count	Difference
Total	5946	5946	0
Passed	1621	1621	0
Failed	4325	4325	0
Panics	0	0	0
Coverage	27.26%	27.26%	0.00%

ts/babel

Test result	main count	This PR count	Difference
Total	588	588	0
Passed	519	519	0
Failed	69	69	0
Panics	0	0	0
Coverage	88.27%	88.27%	0.00%

ts/microsoft

Test result	main count	This PR count	Difference
Total	16257	16257	0
Passed	12395	12395	0
Failed	3862	3862	0
Panics	0	0	0
Coverage	76.24%	76.24%	0.00%

[calibre-analytics]

Comparing feat(rome_js_analyze): noUselessFragment Snapshot #14 to median since last deploy of rome.tools.

	LCP?	CLS?	TBT?
Overall Median across all pages and test profiles	1.89s  from 561ms	0.0 no change	174ms  from 46ms
Chrome Desktop Chrome Desktop • Cable	1.89s  from 610ms	0.0 no change	357ms  from 239ms
iPhone, 4G LTE iPhone 12 • 4G LTE	926ms  from 215ms	0.0 no change	0ms no change
Motorola Moto G Power, 3G connection Motorola Moto G Power • Regular 3G	12.5s  from 561ms	0.0 no change	174ms  from 46ms

1 page tested

 Home

Browser previews

Chrome Desktop	iPhone, 4G LTE	Motorola Moto G Power, 3G connection

Most significant changes

	Value	Budget
JS Parse & Compile iPhone, 4G LTE	448ms  from 8ms
Total JavaScript Size in Bytes Chrome Desktop	4.04 MB  from 86.8 KB
Total JavaScript Size in Bytes iPhone, 4G LTE	4.04 MB  from 86.8 KB
Total JavaScript Size in Bytes Motorola Moto G Power, 3G connection	4.04 MB  from 86.8 KB
JS Parse & Compile Motorola Moto G Power, 3G connection	1.48s  from 31ms

_{28 other significant changes: JS Parse & Compile on Chrome Desktop, First Contentful Paint on Motorola Moto G Power, 3G connection, Largest Contentful Paint on Motorola Moto G Power, 3G connection, Speed Index on Motorola Moto G Power, 3G connection, Total Page Size in Bytes on Chrome Desktop, Total Page Size in Bytes on iPhone, 4G LTE, Total Page Size in Bytes on Motorola Moto G Power, 3G connection, Number of Requests on Motorola Moto G Power, 3G connection, Time to Interactive on Motorola Moto G Power, 3G connection, Number of Requests on Chrome Desktop, Number of Requests on iPhone, 4G LTE, First Contentful Paint on Chrome Desktop, Time to Interactive on Chrome Desktop, First Contentful Paint on iPhone, 4G LTE, Speed Index on Chrome Desktop, Speed Index on iPhone, 4G LTE, Largest Contentful Paint on iPhone, 4G LTE, Total Blocking Time on Motorola Moto G Power, 3G connection, Time to Interactive on iPhone, 4G LTE, Largest Contentful Paint on Chrome Desktop, Total Image Size in Bytes on Chrome Desktop, Total Image Size in Bytes on iPhone, 4G LTE, Total Image Size in Bytes on Motorola Moto G Power, 3G connection, Total HTML Size in Bytes on Chrome Desktop, Total HTML Size in Bytes on iPhone, 4G LTE, Total HTML Size in Bytes on Motorola Moto G Power, 3G connection, Lighthouse Performance Score on Motorola Moto G Power, 3G connection, Lighthouse Performance Score on Chrome Desktop}

_{Calibre: Site dashboard | View this PR | Edit settings | View documentation}

[MichaReiser]

Is Fragment jargon? Is it OK for us to assume that people know that <> is called a fragment or should we change the messaging

[ematipico]

We will have a rule called useFragmentSyntax. Maybe that's where we can give more explanation?

[MichaReiser]

Would you mind adding an example with a comment (and yes, this is valid)

</* test */>empty</>;
<>empty</* comment */ />;
<>empty</ /* comment */>

I would argue that the rule should probably not trigger for these cases.

[ematipico]

The rule as it is now triggers for these cases, it seems. Would you mind give us a reason, please? It seems an edge case, so I can better document the code and rule itself

[MichaReiser]

My reasoning is that these fragments are entirely useless because it is otherwise not possible to add the comments (except using an expression).

However, we could reason that we should flag these nonetheless but not provide an autofix because it is unclear where to place the comment?

[ematipico]

Yeah this rule is not over yet, there are still few cases that doesn't take into account. And the autofix will arrive with another PR :)

[leops]

Does this rule properly handle the case where the Fragment component is user-defined instead of being imported from React ? It seems important that the rule does not raise a diagnostic for this code:

function Fragment() {}
<Fragment>foo</Fragment>

[ematipico]

@leops the last commit adds a fix and test case to cover what you suggested. Nice catch!