🦋 Changeset detected

Latest commit: bec083a

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

The latest updates on your projects. Learn more about Vercel for Git ↗︎

How about biome?

How about biome?

Good question! To be honest, I didn't try Biome yet, but I've heard good things about it. I was simply sticking with ESLint as it's part of my default setup I use for all projects and as it's also used in other Astro repos. Personally, I'm open for adopting Biome if it provides an experience for contributors that is on par with ESLint and finds at least the same issues.

Thank you for the great suggestions! I commented on all of them and applied most of them.

One last question, I see the "Unchanged files with check annotations" in GitHub:

• How does it work?
• Why is it only reporting so few errors?

To my knowledge, this is a beta GitHub feature that's using an error matcher to automatically create annotations from the errors contained in the ESLint CLI output. I don't know why it's only reporting a subset of errors. Maybe the output is limited by default, or the error matcher doesn't recognize some of the ESLint output yet. I wanted to use a different approach with a JSON output file and a separate annotation step at first, but this doesn't work on PRs coming in from forks due to security reasons, so I had to remove this again and not GitHub only applies its default matching logic.

To my knowledge, this is a beta GitHub feature that's using an error matcher to automatically create annotations from the errors contained in the ESLint CLI output. I don't know why it's only reporting a subset of errors. Maybe the output is limited by default, or the error matcher doesn't recognize some of the ESLint output yet. I wanted to use a different approach with a JSON output file and a separate annotation step at first, but this doesn't work on PRs coming in from forks due to security reasons, so I had to remove this again and not GitHub only applies its default matching logic.

I see, thanks for the explanation. I was wondering if you had something special in place for this or not.

How about biome?

I actually just did a test right now, I'm still amazed how crazy fast it is and how much easier the configuration is. By turning off only style rules, we get pretty much to the same number of errors as ESLint, altho unfortunately, the errors that started the idea of this PR are not reported yet. Maybe in the future 🤞

I think configuration and setup wise, we are pretty much close to the minimum of what we would need. What do you think could be a good next step Hippo? Maybe making a list of all errors reported and figuring out a smaller list of obvious rules we want enabled as they are very valuable? And later on discuss the remaining ones?

How about biome?

I actually just did a test right now, I'm still amazed how crazy fast it is and how much easier the configuration is. By turning off only style rules, we get pretty much to the same number of errors as ESLint, altho unfortunately, the errors that started the idea of this PR are not reported yet. Maybe in the future 🤞

Coincidentally I also just did an exploration into Biome! :) I was able to configure it in a way that reports the errors we were looking for. I created #1649 as an alternative to this PR so we can compare.

I think configuration and setup wise, we are pretty much close to the minimum of what we would need. What do you think could be a good next step Hippo? Maybe making a list of all errors reported and figuring out a smaller list of obvious rules we want enabled as they are very valuable? And later on discuss the remaining ones?

My preference would be to decide about the linter to use (ESLint vs Biome), and then go through the errors and actually fix them instead of turning off any further rules. :) In my experience this doesn't take long. Most fixes are straightforward, and there may just be a bit of discussion about how to fix cases in which we have multiple options that all seem to make sense.

I was able to configure it in a way that reports the errors we were looking for.

Ah maybe we are not exactly talking about the exact same error. For me, useAwait only cares about an async function which lacks an await expression.
If we have a test similar to the following one which is reported by both this PR and the biome PR:

test("validates something", async () => {
  expect(async () => await doSomething()).rejects.toThrowError(/invalid/);
});

The big difference for me between useAwait and @typescript-eslint/no-floating-promises is that if my code was instead this one:

test("validates something", async () => {
  await expect(async () => await doSomething1()).rejects.toThrowError(/invalid 1/);
  expect(async () => await doSomething2()).rejects.toThrowError(/invalid 2/);
});

My test does contain an await so useAwait will not report any error. But @typescript-eslint/no-floating-promises will report an error for the second expect call because it is not awaited.

Good catch! Yeah, the no-floating-promises rule doesn't seem to have an equivalent in Biome yet. Also, that the reported issues are not being picked up by GitHub's problem matcher is another downside of Biome right now.

We could just start with ESLint and maybe migrate to Biome once these were fixed?

We could just start with ESLint and maybe migrate to Biome once these were fixed?

I think that's a good approach.

go through the errors and actually fix them instead of turning off any further rules

I personally did not have the time yet to review all the reported errors to make sure they all are real issues in their context as Chris mentioned, make sense for us and would not be too annoying on the long run so if you prefer fixing them all first and we can decide in a later review if we want to disable some rules because the changes are too annoying/not relevant, I'm fine with that 👍

Just a small update to mention that I did not forget about this PR. I still have a local copy of this branch that I update regularly.

Do you want to push that? Or should we close this do you think @HiDeoo? typescript-eslint v8 was released, so I guess we could fix things if we wanted.

Do you want to push that? Or should we close this do you think @HiDeoo? typescript-eslint v8 was released, so I guess we could fix things if we wanted.

I'll update my local branch using a flat config with the latest deps and see where we stand and report back 👍

Finally got the time to get back to this PR and update it, sorry for the extra super long delay!

Here is a summary of the changes I made:

• Updated the branch which was quite old
• Updated ESLint and all related packages
• Switched to a flag config
• Reverted all changes previously made to the PR to ensure some changes due to previous ESLint errors were still relevant now (turns out some of them were not so this was not entirely a huge waste of time ^^)
• Disabled a bit more rules and entirely commented each config change in the ESLint config
• Fixed all the remaining and new errors
• Remove eslint-plugin-no-only-tests as Vitest defaults to error for this on CI and we do the same for Playwright
• Add the ESLint extension to the recommendations in .vscode/extensions.json

I did not add eslint-plugin-astro as we could do that at a later stage when we have more experience with maintaining the codebase with ESLint being part of it and also at the moment, it does not yet support projectService.

I don't think me updating the branch means "ok, we should merge this" but it's more of a "ok, let's see now what this would look like with everything updated, are we happy with this and all the changes, do we need to disable more rules, etc.". I have been so focused on fixing errors that I also think I need some time to step back and look at all the changes too.

Any feedbacks and opinions are welcome of course.

One question: this doesn’t include eslint-plugin-astro. Is that something you think we should consider too for consistency? (Not essential to include in this PR I don’t think necessarily, unless it might inform a bit more which rules to enable/disable.)

Indeed, this is something I commented on in this message:

I did not add eslint-plugin-astro as we could do that at a later stage when we have more experience with maintaining the codebase with ESLint being part of it and also at the moment, it does not yet support projectService.

I think it's highly likely that we will need to tweak rules when adding it, but I think it's fine to do that in the future, once we get used to the current setup, maybe refine it based on usage, and maybe once the plugin supports projectService as well (we can use a different approach for that last point but projectService is the new recommended approach by typescript-eslint).

A question for future us would presumably be whether we ever move to Biome for linting and formatting once full Astro support is available, but that could be a way off.

I think that would be the ultimate goal (which was even tested in #1649) but we would indeed need full Astro support and also no-floating-promises as I commended on in this message, as this rule was part of what initially triggered the initiative.

Just updated the PR, updated some new dependencies introduced in this PR, fixed the new issues following the branch update, and addressed the last review (the most notable change is createTranslationSystem now being async as my assumption about i18n.init() was based on older i18next versions and no longer valid).

Thanks @hippotastic for kicking this off in the distant mists of time and to @HiDeoo for forging ahead to take us to the promised lint.

See you in 6 months when we rewrite it to use Biome 😁

Hahaha, I just checked the date when I first opened this - it really has been a while! Amazing to see how the rough 4-day draft turned into such a solid setup. Huge thanks to @HiDeoo for taking the torch and shaping it into something that truly fits Starlight's codebase.

Can't wait for noFloatingPromises to leave Biome V2's nursery group, then we might actually finally revisit #1649. 😄 And given the solid setup we now have thanks to you, it hopefully won't take another year.