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 noUnwantedPolyfillio #4731

Open
wants to merge 11 commits into
base: next
Choose a base branch
from

Conversation

unvalley
Copy link
Member

@unvalley unvalley commented Dec 12, 2024

Summary

Implement no-unwanted-polyfillio from eslint-plugin-next.

Test Plan

Add snapshot tests

@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project A-Linter Area: linter L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis A-Changelog Area: changelog labels Dec 12, 2024
@unvalley unvalley force-pushed the no-unwanted-polyfillio branch from bd144d1 to 91c936b Compare December 14, 2024 07:50
Copy link

codspeed-hq bot commented Dec 14, 2024

CodSpeed Performance Report

Merging #4731 will not alter performance

Comparing unvalley:no-unwanted-polyfillio (057c0ed) with next (a478377)

Summary

✅ 97 untouched benchmarks

@unvalley unvalley marked this pull request as ready for review December 14, 2024 08:33
Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

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

Nice!

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.

I would appreciate if you could point this PR to next. The majority of new code and features are there. We recently started changing how the analyzer works, and we added the domains. Among domains, we have a "next" domain, and this rule should be added to that.

use biome_js_syntax::JsImport;
use biome_rowan::AstNode;

/// Represent Next.js libraries.
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain what "next libraries" means? The implementation shows a next/script, which isn't a library. It's a specifier of the next package, however this comment seems misleading, because I understood external libraries.

Copy link
Member Author

@unvalley unvalley Dec 15, 2024

Choose a reason for hiding this comment

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

sorry you are right, how about call them "Next.js built-in utility(utilities)"? I would like to point to next/image, next/link etc.

Copy link
Member

Choose a reason for hiding this comment

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

That's definitely better! Good suggestion

@unvalley unvalley changed the base branch from main to next December 15, 2024 15:29
@unvalley unvalley force-pushed the no-unwanted-polyfillio branch from de9fa96 to f0feb37 Compare December 15, 2024 17:42
@unvalley unvalley force-pushed the no-unwanted-polyfillio branch from af49062 to e59429c Compare December 15, 2024 18:13
@unvalley unvalley requested a review from ematipico December 16, 2024 10:05
Comment on lines 49 to 56
pub NoUnwantedPolyfillio {
version: "next",
name: "noUnwantedPolyfillio",
language: "jsx",
sources: &[RuleSource::EslintNext("no-unwanted-polyfillio")],
source_kind: RuleSourceKind::SameLogic,
recommended: false,
}
Copy link
Member

Choose a reason for hiding this comment

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

In case you missed the new features, here the domains.

For this rule, we should assign the RuleDomain::Next. And recommend the rule.

Also, we should assign a proper severity. I leave it to you based on your knowledge.

Plus, we should add a changeset.

Apologies for letting you change things again. Things have been changing a lot :)

Copy link
Member Author

Choose a reason for hiding this comment

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

love the domains feature! I've addressed them at cc081f8 and 057c0ed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-CLI Area: CLI A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Project Area: project L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants