chore: use Biome as linter #111
Conversation
|
There was a problem hiding this comment.
@ematipico Thanks for taking the time and helping with this!
I'm in favor of this and thing it is great. 🚀
All the changes lgtm!
However, I would like to see that we replace eslint completely. Can you give some more reasons, why we still need eslint?
| "enabled": true, | ||
| "rules": { | ||
| "suspicious": { | ||
| "noConsoleLog": "off", |
There was a problem hiding this comment.
Can we still get a warn?
There was a problem hiding this comment.
You would get a bunch of warnings, which are annoying I felt annoying. Ideally, we could use an override, but they are currently buggy. Are you still happy with a warning?
There was a problem hiding this comment.
Hm that is a tricky one. In general we don't really want to use console.log() for anything user-facing, everything user-facing should probably use astro's logger or error. So getting a warning, sounds feasible. However I was also annoyed by it before, so disabling this makes also sense.
I'm happy both ways, so maybe keep it off for now!
| "rules": { | ||
| "suspicious": { | ||
| "noConsoleLog": "off", | ||
| "noExplicitAny": "off" |
There was a problem hiding this comment.
What is the reason to disable this rule?
There was a problem hiding this comment.
There are a bunch of any that I didn't want to touch, so I turned off the rule. How do you want to proceed?
There was a problem hiding this comment.
Oh I didn't know we had them :/
Let's keep the rule off for now, and I'll double check the code, to get rid of those anys
packages/netlify/src/ssr-function.ts
Outdated
| @@ -21,6 +21,7 @@ export const createExports = (manifest: SSRManifest, _args: Args) => { | |||
| let locals: Record<string, unknown> = {}; | |||
|
|
|||
| if (request.headers.has('x-astro-locals')) { | |||
| // biome-ignore lint/style/noNonNullAssertion: safe because we checked before | |||
There was a problem hiding this comment.
I would love if typescript would be able to handle those checks on Maps....
There was a problem hiding this comment.
I can change the code. The .get method returns undefined, so we can check the result. We wouldn't need the bang operator
There was a problem hiding this comment.
Done in 0b9fc0e (#111)
There was a problem hiding this comment.
Oh yeah that would work, I think rewriting the code makes sense.
I just think Typescript should support Map's .has()
Unfortunately, Biome linter isn't on par with |
Alright, sounds good for me. I really would love to get rid of eslint, I wonder how valuable those rules with type information are, but nothing to worry about in this PR! |
One that I deem really important is: https://typescript-eslint.io/rules/no-floating-promises/ |
Changes
This PR uses Biome as linter. I applied safe and unsafe fixes. I suppressed some rules where we don't have enough information or where things were already suppressed with eslint.
Testing
Current CI should work
Docs
N/A