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(noUndeclaredVariables): add checkTypes option #4471

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented Nov 5, 2024

Summary

See this thread for some context.

Add a new checkTypes option for noUndeclaredVariables.

The option is turned on by default. I plan to turn it off in Biome 2.0.
This will bring noUndeclaredVariables closer to no-undef (So we will set the rule as a source instead of an inspiration).
This also eliminates many false positives.
TypeScript is better suited to perform this kind of check.
We could consider in Biome 2.0 to turn on the rule by default because checkTypes set to false will reduce significantly the number of false positives.

I chose to name the option checkTypes instead of ignoreTypes because I plan to make it false by default.

Test Plan

I added a test.

@Conaclos Conaclos requested review from a team November 5, 2024 13:48
@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages A-Changelog Area: changelog labels Nov 5, 2024
@Conaclos Conaclos force-pushed the conaclos/noUndeclaredVariables-check-types branch from c7d12ea to 464e9e3 Compare November 5, 2024 13:50
Copy link

codspeed-hq bot commented Nov 5, 2024

CodSpeed Performance Report

Merging #4471 will improve performances by 6.1%

Comparing conaclos/noUndeclaredVariables-check-types (1addede) with main (3ad6da5)

Summary

⚡ 1 improvements
✅ 98 untouched benchmarks

Benchmarks breakdown

Benchmark main conaclos/noUndeclaredVariables-check-types Change
react.production.min_3378072959512366797.js[uncached] 2.2 ms 2.1 ms +6.1%

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.

We could consider in Biome 2.0 to turn on the rule by default

Maybe for 2.0 we can do a check in biome init to see if there is a tsconfig.json? If there is then we disable the rule since it would be redundant with TS, but if there isn’t we enable it.

It kinda makes me wonder if Biome 2.0 should even have the checkTypes option at all. If you use TypeScript, it takes care of it. If you don’t, you don’t have types anyway.

@ematipico
Copy link
Member

It kinda makes me wonder if Biome 2.0 should even have the checkTypes option at all. If you use TypeScript, it takes care of it. If you don’t, you don’t have types anyway.

True, but people complain that TypeScript is slow. If Biome can do what TypeScript does already, but faster and without losing performance, I'd say it's a win for Biome :)

@arendjr
Copy link
Contributor

arendjr commented Nov 6, 2024

If Biome can do what TypeScript does already, but faster and without losing performance, I'd say it's a win for Biome :)

I wholeheartedly agree, and it's a great argument for building our own type checker, but this option is not coming even near that 😅

@Conaclos Conaclos force-pushed the conaclos/noUndeclaredVariables-check-types branch 2 times, most recently from f47cb41 to 1addede Compare November 6, 2024 13:05
@Conaclos Conaclos merged commit d9d7bd7 into main Nov 6, 2024
11 checks passed
@Conaclos Conaclos deleted the conaclos/noUndeclaredVariables-check-types branch November 6, 2024 13:38
@kurtextrem
Copy link

Thank you!

@elawad
Copy link

elawad commented Dec 10, 2024

Hi, I tested the nightly build v1.9.5-nightly.81fdedb
But getting an error running Biome. Is the schema up to date, or did I miss something?

✖ options has an incorrect type, expected no value, but received an object.

  24 │         "noUndeclaredVariables": {
  25 │           "level": "error",
> 26 │           "options": { "checkTypes": false }
     │                      ^^^^^^^^^^^^^^^^^^^^^^^
  27 │         },

@Conaclos
Copy link
Member Author

@elawad. I've just tested with the nightly and everything is ok for me. Please make sure you use the latest nightly (and check with npx @biomejs/biome --version. Otherwise, open an issue with a reproduction.

@elawad
Copy link

elawad commented Dec 11, 2024

Thanks, please disregard. It was an error on my part. Needed to uninstall first, then install the nightly.

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.

6 participants