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(analyze): check if a fragment has only one child element using tokens. #3582

Merged

Conversation

satojin219
Copy link
Contributor

Summary

This bug occurs when a warning is supposed to be issued if a fragment has only one child element, but if line breaks or tabs are included, the warning does not appear. To check the number of child elements, child_list.len() is used, but this includes line breaks and tabs, causing the count of child elements to be more than one.
https://github.com/biomejs/biome/blob/main/crates/biome_js_analyze/src/lint/complexity/no_useless_fragments.rs#L165

Therefore, I modified the code to scan the tokens of the fragment's children and check if there is only one JsSyntaxKind::JSX_SELF_CLOSING_ELEMENT or JsSyntaxKind::JSX_ELEMENT.

Test Plan

Change the path on line 17 of crates/biome_js_analyze/tests/quick_test.rs to tests/specs/complexity/noUselessFragments/issue_3545.jsx and tests/specs/complexity/noUselessFragments/withJsxElementInvalid.jsx. Run the tests and confirm that both tests pass successfully.

closed #3545

@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Aug 4, 2024
Copy link

codspeed-hq bot commented Aug 4, 2024

CodSpeed Performance Report

Merging #3582 will degrade performances by 6.98%

Comparing satojin219:fix/no-useless-fragments-one-element (fcd861c) with main (10ef9d4)

Summary

❌ 1 regressions
✅ 103 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main satojin219:fix/no-useless-fragments-one-element Change
router_14177007973872119684.ts[cached] 2.1 ms 2.2 ms -6.98%

@Conaclos Conaclos changed the title fix(analyze): Check if a fragment has only one child element using tokens. fix(analyze): check if a fragment has only one child element using tokens. Aug 4, 2024
@Conaclos
Copy link
Member

Conaclos commented Aug 4, 2024

A test i s failing.
According to it the following code shoudl also be rreporetd as useless:

<>
foo
</>

Comment on lines 34 to 38
2 │ - <>
3 │ - → <Component·/>
4 │ - </>;
2 │ + "";
5 3 │
Copy link
Member

Choose a reason for hiding this comment

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

There's a regression here

@satojin219
Copy link
Contributor Author

@Conaclos @ematipico
I have fixed the bug, so please review again.

@ematipico
Copy link
Member

@github-actions github-actions bot added the A-Changelog Area: changelog label Aug 13, 2024
@satojin219
Copy link
Contributor Author

@ematipico
#3582 (comment)

chore: updaste CHANGELOG.md
I've added it to the changelog!

@ematipico ematipico merged commit ecbc627 into biomejs:main Aug 13, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Linter Area: linter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

💅 noUselessFragments doesn't catch one-child case that's caught in eslint react/jsx-no-useless-fragment
3 participants