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(eslint-config-fluid): Add strict biome-compatible config #22960

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tylerbutler
Copy link
Member

@tylerbutler tylerbutler commented Nov 1, 2024

Adds a strict config to the shared eslint package that can be used in projects to use Biome for linting in conjunction with eslint. This config is the same as the strict config, but disables rules that have a Biome equivalent.

To make it easier to see the differences, I also updated the strict config - this will not be merged.

@github-actions github-actions bot added area: build Build related issues base: main PRs targeted against main branch labels Nov 1, 2024
@@ -20,7 +20,7 @@ module.exports = {
es2024: false,
node: true,
},
extends: ["./recommended.js"],
extends: ["./recommended.js", "biome"],
Copy link
Member Author

Choose a reason for hiding this comment

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

Do not merge without reverting this.

Copy link
Contributor

Choose a reason for hiding this comment

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

So why isn't it reverted?

Copy link
Member Author

Choose a reason for hiding this comment

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

I made this change so it was easier to see the diff between the strict and strict-biome configs via the printed config. Once reviewed I'll revert.

@tylerbutler tylerbutler requested review from a team, Josmithr and jikim-msft November 1, 2024 16:16
Copy link
Contributor

@jason-ha jason-ha left a comment

Choose a reason for hiding this comment

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

Couldn't we just disable formatting things generally and leave it up to formatters directly?

@@ -24,6 +24,7 @@
"print-config:react": "eslint --config ./index.js --print-config ./src/file.tsx > ./printed-configs/react.json",
"print-config:recommended": "eslint --config ./recommended.js --print-config ./src/file.ts > ./printed-configs/recommended.json",
Copy link
Contributor

Choose a reason for hiding this comment

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

No interaction in recommended?

@@ -20,7 +20,7 @@ module.exports = {
es2024: false,
node: true,
},
extends: ["./recommended.js"],
extends: ["./recommended.js", "biome"],
Copy link
Contributor

Choose a reason for hiding this comment

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

So why isn't it reverted?

@tylerbutler
Copy link
Member Author

Couldn't we just disable formatting things generally and leave it up to formatters directly?

This change is not about formatting. It's about linting. Eslint 8 is deprecated. Eslint 9 will require work to adopt. I want to explore not using Eslint altogether, or supplementing it with a faster linter (biome, oxlint) to better inform our lint plans for the future.

@jason-ha
Copy link
Contributor

jason-ha commented Nov 8, 2024

Couldn't we just disable formatting things generally and leave it up to formatters directly?

This change is not about formatting. It's about linting. Eslint 8 is deprecated. Eslint 9 will require work to adopt. I want to explore not using Eslint altogether, or supplementing it with a faster linter (biome, oxlint) to better inform our lint plans for the future.

By "formatting things" I mean all lint formatting rules. Suggesting that we don't have a special Biome config and instead disable all of these format related rules everywhere.

@tylerbutler
Copy link
Member Author

tylerbutler commented Nov 8, 2024

By "formatting things" I mean all lint formatting rules. Suggesting that we don't have a special Biome config and instead disable all of these format related rules everywhere.

Maybe I am misunderstanding. The special Biome config is not disabling formatting rules exclusively. It's disabling lint rules from this list (partially - only the "recommended" rules), which includes stuff like no explicit-any and use-for-of.

If you look at the strict printed config diff in this PR you can see all the rules that are getting disabled: https://github.com/microsoft/FluidFramework/pull/22960/files#diff-7c530cc3edf251a186efaba26d97f51273af07d4fc094e7f2ea362a5b0fdb742

@jason-ha
Copy link
Contributor

jason-ha commented Nov 8, 2024

By "formatting things" I mean all lint formatting rules. Suggesting that we don't have a special Biome config and instead disable all of these format related rules everywhere.

Maybe I am misunderstanding. The special Biome config is not disabling formatting rules exclusively. It's disabling lint rules from this list (partially - only the "recommended" rules), which includes stuff like no explicit-any and use-for-of.

If you look at the strict printed config diff in this PR you can see all the rules that are getting disabled: https://github.com/microsoft/FluidFramework/pull/22960/files#diff-7c530cc3edf251a186efaba26d97f51273af07d4fc094e7f2ea362a5b0fdb742

Hmm. I think the diffs might have been weird when #22959 was outstanding. And I think the title can be interpreted two ways.

Could be nice to add some notes to the readme about how to use the new config. I am guessing that Biome needs to be used in conjunction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: build Build related issues base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants