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(lint): add missing domain to next.js rules #5046

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

Conversation

Jayllyz
Copy link
Contributor

@Jayllyz Jayllyz commented Feb 6, 2025

Summary

Closes #5045

Test Plan

CI

@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Feb 6, 2025
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.

Good catch! However, this is a new feature that we haven't released yet, so having a fix changeset doesn't make sense :)

Could you add a new CLI test to prevent this regression?

@Jayllyz
Copy link
Contributor Author

Jayllyz commented Feb 6, 2025

Good catch! However, this is a new feature that we haven't released yet, so having a fix changeset doesn't make sense :)

Ok, I'll remove it.

Could you add a new CLI test to prevent this regression?

I'm sorry, I don't understand what exactly I need to test.

@ematipico
Copy link
Member

ematipico commented Feb 6, 2025

I'm sorry, I don't understand what exactly I need to test.

So, the nice thing of domains is that Biome can enable a rule automatically by checking the dependencies of a package.json project. So, in this PR should we recommend: true these rules, and they will be automatically enabled only if the project has next in its dependencies.

Then, we can create a CLI test like this one:

#[test]
fn enables_react_rules_via_dependencies() {
let mut console = BufferConsole::default();
let mut fs = MemoryFileSystem::default();
let file_path = Utf8Path::new("package.json");
fs.insert(
file_path.into(),
r#"{
"dependencies": {
"react": "^16.0.0"
}
}
"#
.as_bytes(),
);
let content = r#"
import { useCallback } from "react";
function Component2() {
const [local,SetLocal] = useState(0);
const handle = useCallback(() => {
console.log(local);
}, [local, local]);
}
"#;
let test = Utf8Path::new("test.jsx");
fs.insert(test.into(), content.as_bytes());
let (fs, result) = run_cli(
fs,
&mut console,
Args::from(["lint", test.as_str()].as_slice()),
);
assert!(result.is_err(), "run_cli returned {result:?}");
assert_cli_snapshot(SnapshotPayload::new(
module_path!(),
"enables_react_rules_via_dependencies",
fs,
console,
result,
));
}

We put next as a dependency, and we add some file that violates one of the rules you're updating. The snapshot should contain a diagnostic of the lint rule we're testing.

Copy link

codspeed-hq bot commented Feb 6, 2025

CodSpeed Performance Report

Merging #5046 will not alter performance

Comparing Jayllyz:fix/missing-next-domain (81a6d2e) with next (2696970)

Summary

✅ 94 untouched benchmarks

@github-actions github-actions bot added the A-CLI Area: CLI label Feb 6, 2025
@Jayllyz
Copy link
Contributor Author

Jayllyz commented Feb 6, 2025

I've pushed the new Next.js test, I can't find out why the rules aren't activated, if I create a biome.json that manually activates the rule, it works.

@ematipico
Copy link
Member

ematipico commented Feb 6, 2025

@Jayllyz did you run just gen-analyzers? There's some codegen that is involved with the runtime of the rules. That's probably why it isn't working.

@Jayllyz
Copy link
Contributor Author

Jayllyz commented Feb 6, 2025

@Jayllyz did you run just gen-analyzers? There's some codegen that is involved with the runtime of the rules. That's probably why it isn't working.

Oh yes forgot the codegen, i jst did just gen-all and still the generated snapshot don't detect the error and it also broke one more test (enables_all_rules_when_group_is_error).

@Jayllyz Jayllyz force-pushed the fix/missing-next-domain branch from 7a384b0 to ab8a360 Compare February 6, 2025 16:09
@github-actions github-actions bot added the A-Project Area: project label Feb 6, 2025
@ematipico
Copy link
Member

Thank you @Jayllyz, I'll look into it. Just so I understand, what's the expected rule to be triggered by your test?

@Jayllyz
Copy link
Contributor Author

Jayllyz commented Feb 6, 2025

Thank you @Jayllyz, I'll look into it. Just so I understand, what's the expected rule to be triggered by your test?

This one https://biomejs.dev/linter/rules/no-img-element

@ematipico
Copy link
Member

@Jayllyz I fixed the problem. The original rule doesn't have any Severity, this means that its default severity is Information, which doesn't emit any error. I changed is_err to is_ok (same as the other tests 😛 ). It passes now

@ematipico
Copy link
Member

Found the bug!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI 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.

2 participants