feat(analyze): implement noSyncScripts
#8108
Conversation
🦋 Changeset detectedLatest commit: 01b484a The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
A-CLI
Netail
force-pushed
the
feat/no-sync-scripts
branch
from
81cda61 to
2b04613
Compare
WalkthroughAdds a new nursery lint rule Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)crates/biome_js_analyze/src/lint/nursery/no_sync_scripts.rs (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
🔇 Additional comments (7)
Comment |
CodSpeed Performance ReportMerging #8108 will not alter performanceComparing Summary
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/biome_js_analyze/src/lint/nursery/no_sync_scripts.rs (3)
11-52: Docs and diagnostic text are clear; one tiny wording tweakThe rule metadata and diagnostic message align well with
@next/next/no-sync-scriptsand explain both the problem and the fix. If you fancy polishing the copy, consider changing the docstring sentence:A synchronous script which impact your webpage performanceto something like:
A synchronous script can impact your webpage performanceto read more naturally in English, but that’s purely cosmetic.
Also applies to: 109-121
88-107: Optional: factor out shared logic between the two match armsBoth
JsxOpeningElementandJsxSelfClosingElementarms perform the same sequence ofname+attributes+ validation calls; you could pull that into a small helper likecheck_script_like(&node.name().ok()?, &node.attributes())to reduce duplication, but it’s not critical.
61-80: Match Next.js semantics by handling JSX spread attributesVerified: Next.js intentionally allows
<script {...props} />because spreads might supplyasyncordefer, avoiding false positives. Your current implementation flags<script src={url} {...props} />as synchronous sincefind_by_namecan't see into spreads.To align with upstream behaviour:
- Early-return
Noneinvalidate_attributeswhen the attribute list contains a JSX spread- Add a test fixture (e.g.
<script src="" {...props} />) to lock this inThis keeps Biome's rule safely aligned with Next.js whilst remaining conservative.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rsis excluded by!**/migrate/eslint_any_rule_to_biome.rsand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_configuration/src/generated/domain_selector.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/src/lint/nursery.rsis excluded by!**/nursery.rsand included by**crates/biome_js_analyze/tests/specs/nursery/noSyncScripts/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noSyncScripts/valid.jsx.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (6)
.changeset/icy-bags-sleep.md(1 hunks)crates/biome_js_analyze/src/lint/nursery/no_sync_scripts.rs(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noSyncScripts/invalid.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noSyncScripts/valid.jsx(1 hunks)crates/biome_rule_options/src/lib.rs(1 hunks)crates/biome_rule_options/src/no_sync_scripts.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/lint/nursery/no_sync_scripts.rs (1)
crates/biome_analyze/src/rule.rs (4)
recommended(602-605)sources(617-620)same(246-251)domains(632-635)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Documentation
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Check Dependencies
- GitHub Check: Check JS Files
- GitHub Check: Test Node.js API
- GitHub Check: autofix
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Bench (biome_js_formatter)
🔇 Additional comments (5)
crates/biome_rule_options/src/no_sync_scripts.rs (1)
1-6: Options struct looks consistentDerives and serde attributes match the usual pattern for unit options structs, so this is ready to go as-is.
crates/biome_rule_options/src/lib.rs (1)
186-186: Module export wiring is fineThe new
no_sync_scriptsmodule is correctly exposed alongside the otherno_*rule options; no further changes needed.crates/biome_js_analyze/tests/specs/nursery/noSyncScripts/invalid.jsx (1)
1-3: Invalid case matches the rule semanticsThis is a clear minimal repro for the “src without async/defer” case and should reliably trigger the diagnostic.
.changeset/icy-bags-sleep.md (1)
1-18: Changeset description is aligned and usefulThe summary and JSX examples clearly describe what the rule flags and how to fix it, matching the Next.js guidance, so the metadata looks good.
crates/biome_js_analyze/tests/specs/nursery/noSyncScripts/valid.jsx (1)
1-16: Good spread of non-diagnostic casesThese fixtures exercise the key safe patterns (no
src, andsrcwithasync/defer) and should keep the rule from being over-eager on common JSX.
Netail
force-pushed
the
feat/no-sync-scripts
branch
from
2b04613 to
8bc4f99
Compare
Netail
changed the title
noSyncScriptsnoSyncScripts
github-actions
bot
added
A-Parser
| name: "noSyncScripts", | ||
| language: "html", | ||
| recommended: false, | ||
| sources: &[RuleSource::EslintNext("no-sync-scripts").same()], |
There was a problem hiding this comment.
Is this relevant for the html analyze? Perhaps inspired?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/biome_html_syntax/src/attr_ext.rs (1)
22-34: Consider removing redundantNone.The explicit
Noneat line 31 is unnecessary—find_maphandles non-matches when the closure simply doesn't return.Apply this diff:
impl HtmlAttributeList { pub fn find_by_name(&self, name_to_lookup: &str) -> Option<HtmlAttribute> { self.iter().find_map(|attribute| { if let AnyHtmlAttribute::HtmlAttribute(attribute) = attribute && let Ok(name) = attribute.name() && name.value_token().ok()?.text_trimmed() == name_to_lookup { - return Some(attribute); + Some(attribute) + } else { + None } - None }) } }Or more idiomatically:
impl HtmlAttributeList { pub fn find_by_name(&self, name_to_lookup: &str) -> Option<HtmlAttribute> { self.iter().find_map(|attribute| { - if let AnyHtmlAttribute::HtmlAttribute(attribute) = attribute + let AnyHtmlAttribute::HtmlAttribute(attribute) = attribute else { + return None; + }; + if let Ok(name) = attribute.name() && let Ok(name) = attribute.name() && name.value_token().ok()?.text_trimmed() == name_to_lookup { - return Some(attribute); + Some(attribute) + } else { + None } - None }) } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
crates/biome_html_analyze/src/lint/nursery.rsis excluded by!**/nursery.rsand included by**crates/biome_html_analyze/tests/specs/nursery/noSyncScripts/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noSyncScripts/valid.html.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (6)
crates/biome_html_analyze/src/lint/nursery/no_sync_scripts.rs(1 hunks)crates/biome_html_analyze/tests/specs/nursery/noSyncScripts/invalid.html(1 hunks)crates/biome_html_analyze/tests/specs/nursery/noSyncScripts/valid.html(1 hunks)crates/biome_html_syntax/src/attr_ext.rs(2 hunks)crates/biome_js_analyze/src/lint/nursery/no_sync_scripts.rs(1 hunks)justfile(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
crates/biome_html_syntax/src/attr_ext.rs (2)
crates/biome_html_syntax/src/lib.rs (1)
inner_string_text(119-128)crates/biome_html_syntax/src/element_ext.rs (1)
name(55-70)
crates/biome_html_analyze/src/lint/nursery/no_sync_scripts.rs (2)
crates/biome_analyze/src/rule.rs (1)
same(246-251)crates/biome_js_analyze/src/lint/nursery/no_sync_scripts.rs (2)
run(89-108)diagnostic(110-124)
crates/biome_js_analyze/src/lint/nursery/no_sync_scripts.rs (2)
crates/biome_analyze/src/rule.rs (4)
recommended(602-605)sources(617-620)same(246-251)domains(632-635)crates/biome_html_analyze/src/lint/nursery/no_sync_scripts.rs (2)
run(44-62)diagnostic(64-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: End-to-end tests
- GitHub Check: Check Dependencies
- GitHub Check: Documentation
- GitHub Check: Test Node.js API
- GitHub Check: Check JS Files
- GitHub Check: autofix
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_configuration)
🔇 Additional comments (6)
justfile (1)
147-147: LGTM!Properly extends test coverage for the HTML analyzer, consistent with the existing pattern.
Also applies to: 152-152
crates/biome_js_analyze/src/lint/nursery/no_sync_scripts.rs (1)
55-59: LGTM!Correctly covers both JSX opening and self-closing script elements.
crates/biome_html_analyze/tests/specs/nursery/noSyncScripts/invalid.html (1)
1-1: LGTM!Appropriate minimal test case for the diagnostic.
crates/biome_html_analyze/tests/specs/nursery/noSyncScripts/valid.html (1)
1-8: LGTM!Comprehensive coverage of valid cases: non-script elements, inline scripts, and properly async/deferred external scripts.
crates/biome_html_analyze/src/lint/nursery/no_sync_scripts.rs (2)
44-62: LGTM!Clean validation logic: checks for script element, requires src, and ensures async/defer are absent.
64-78: LGTM!Clear diagnostic with actionable guidance. Correctly omits Next.js-specific advice for the HTML context.
| fn run(ctx: &RuleContext<Self>) -> Self::Signals { | ||
| let binding = ctx.query(); | ||
|
|
||
| match binding { | ||
| NoSyncScriptsQuery::JsxOpeningElement(node) => { | ||
| let name = node.name().ok()?; | ||
| validate_name(&name)?; | ||
|
|
||
| let attributes = node.attributes(); | ||
| validate_attributes(&attributes) | ||
| } | ||
| NoSyncScriptsQuery::JsxSelfClosingElement(node) => { | ||
| let name = node.name().ok()?; | ||
| validate_name(&name)?; | ||
|
|
||
| let attributes = node.attributes(); | ||
| validate_attributes(&attributes) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Eliminate duplication in match branches.
Both match arms perform identical operations. Extract common logic.
Apply this diff:
fn run(ctx: &RuleContext<Self>) -> Self::Signals {
let binding = ctx.query();
-
- match binding {
- NoSyncScriptsQuery::JsxOpeningElement(node) => {
- let name = node.name().ok()?;
- validate_name(&name)?;
-
- let attributes = node.attributes();
- validate_attributes(&attributes)
- }
- NoSyncScriptsQuery::JsxSelfClosingElement(node) => {
- let name = node.name().ok()?;
- validate_name(&name)?;
-
- let attributes = node.attributes();
- validate_attributes(&attributes)
- }
- }
+ let name = binding.name().ok()?;
+ validate_name(&name)?;
+
+ let attributes = binding.attributes();
+ validate_attributes(&attributes)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn run(ctx: &RuleContext<Self>) -> Self::Signals { | |
| let binding = ctx.query(); | |
| match binding { | |
| NoSyncScriptsQuery::JsxOpeningElement(node) => { | |
| let name = node.name().ok()?; | |
| validate_name(&name)?; | |
| let attributes = node.attributes(); | |
| validate_attributes(&attributes) | |
| } | |
| NoSyncScriptsQuery::JsxSelfClosingElement(node) => { | |
| let name = node.name().ok()?; | |
| validate_name(&name)?; | |
| let attributes = node.attributes(); | |
| validate_attributes(&attributes) | |
| } | |
| } | |
| } | |
| fn run(ctx: &RuleContext<Self>) -> Self::Signals { | |
| let binding = ctx.query(); | |
| match binding { | |
| NoSyncScriptsQuery::JsxOpeningElement(node) | NoSyncScriptsQuery::JsxSelfClosingElement(node) => { | |
| let name = node.name().ok()?; | |
| validate_name(&name)?; | |
| let attributes = node.attributes(); | |
| validate_attributes(&attributes) | |
| } | |
| } | |
| } |
2 participants
Summary
Implement NextJS'
noSyncScripts, but could also be valid for regular React. So made Next.js a kind of sub category, not sure if there's a better way to mention itCloses #7647
Test Plan
Docs