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

feat(lint): add useGoogleFontPreconnect rule #4185

Merged
merged 7 commits into from
Oct 27, 2024

Conversation

kaioduarte
Copy link
Contributor

Summary

Implement google-font-preconnect from eslint-plugin-next.

Test Plan

Snapshots + CI

@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project A-Linter Area: linter A-Parser Area: parser L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis labels Oct 6, 2024
@kaioduarte kaioduarte force-pushed the google-font-preconnect branch from c06b936 to fdaaf79 Compare October 6, 2024 08:42
Copy link

codspeed-hq bot commented Oct 6, 2024

CodSpeed Performance Report

Merging #4185 will not alter performance

Comparing kaioduarte:google-font-preconnect (eeb1d24) with main (6c209ef)

Summary

✅ 101 untouched benchmarks

@ematipico
Copy link
Member

@kaioduarte a suggestion for this and other PRs. When filling out the template, please tell how you named the rule inside Biome.

Since Biome has a strict naming convention, it's often difficult to understand which rule you implemented.

@kaioduarte kaioduarte changed the title feat(lint): add google-font-preconnect from eslint-plugin-next feat(lint): add useGoogleFontPreconnect Oct 6, 2024
@kaioduarte kaioduarte changed the title feat(lint): add useGoogleFontPreconnect feat(lint): add useGoogleFontPreconnect rule Oct 6, 2024
@kaioduarte kaioduarte force-pushed the google-font-preconnect branch from fdaaf79 to 852870e Compare October 7, 2024 10:24
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

The diagnostic must match our rule pillars

@kaioduarte kaioduarte force-pushed the google-font-preconnect branch from 852870e to 2b9ca81 Compare October 12, 2024 13:59
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Thank you! I suggested few changes. Once addressed, we can merge the PR

Comment on lines +101 to +103
if node.find_attribute_by_name("rel").is_some() {
return None;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this check needed? The check was already done in run. Did I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I didn't want to change the value if a rel already exists, maybe we could do so, I'm not sure :)

The action is a bonus since the original rule doesn't have one.

Comment on lines 73 to 75
let rel = node.get_attribute_inner_string_text("rel");

if href.starts_with("https://fonts.gstatic.com") && (rel.is_none() || rel? != "preconnect")
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
let rel = node.get_attribute_inner_string_text("rel");
if href.starts_with("https://fonts.gstatic.com") && (rel.is_none() || rel? != "preconnect")
let rel = node.get_attribute_inner_string_text("rel")?;
if href.starts_with("https://fonts.gstatic.com") && rel != "preconnect"

Since rel is mandatory attribute, we can simplify the check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not mandatory, we emit a diagnostic in the absence of the property or if its value is different of preconnect.

Copy link
Member

@ematipico ematipico Oct 26, 2024

Choose a reason for hiding this comment

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

Then you shouldn't use rel?, because the try operator ? exit from the function in case is None, which goes in conflict with the accepted value rel.is_none. I suggest using matches!(rel, Some("preconnect")) instead.

While it's true we're using an OR operation, it would fail if you flip the arguments

@kaioduarte kaioduarte force-pushed the google-font-preconnect branch from 2b9ca81 to b73296d Compare October 25, 2024 22:29
@ematipico ematipico merged commit d02e3c1 into biomejs:main Oct 27, 2024
13 checks passed
@kaioduarte kaioduarte deleted the google-font-preconnect branch October 27, 2024 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Parser Area: parser A-Project Area: project L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants