Add basic ESLint config, lint script and lint CI job

[hippotastic]

Description

• Adds ESLint to the Starlight repo to help catch non-recommended code patterns and bugs. It can be run locally using pnpm lint from the repo root.
• Adds a lint job to the CI workflow that should generate annotations on PRs to make it easier to see any found issues in context.

[changeset-bot]

⚠️ No Changeset found

Latest commit: bf65c26

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[vercel]

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

Name	Status	Preview	Updated (UTC)
starlight	✅ Ready (Inspect)	Visit Preview	Mar 22, 2024 1:49pm

[liruifengv]

How about biome?

[hippotastic]

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.

[HiDeoo]

Did not have time yet to deep dive in the reported errors individually so it's mostly a general review of the approach and the config but I thought I would still share it to get the discussion started.

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?

[hippotastic]

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.

[HiDeoo]

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?

[hippotastic]

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.

[HiDeoo]

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.

[hippotastic]

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?

[HiDeoo]

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 👍

[HiDeoo]

I still did not get a chance to give this a thorough review but I'd like to point the following recent news:

• ESLint v9 has now been released.
• The new flat config format is now the default one.
• The old format (used in this PR) is now considered deprecated.
• The VSCode ESLint extension is in pre-release stage of a new major which already deprecate eslint.experimental.useFlatConfig in favor of using flat config by default (with a eslint.useFlatConfig setting to opt-out).
• The plugins used in this PR are all already flat config compatible or work out of the box with the new format.
• Blu is already looking into updating Astro setup to v9

Considering all this, we should consider if it may be wise to switch to the new format before landing this PR to avoid having to do it again later and having to introduce in this PR changes to use the now deprecated format (the CLI would need env vars to opt-out of the new format, the VSCode extension would need to be configured to use the old format, etc.)

We may need to wait a bit for the VSCode ESLint extension to release its new major, in the meantime some plugins will release new versions with minor fixes for v9 unrelated changes, but except for that, this should be a pretty easy change (and something I can help if needed, done it already in a few projects including my shareable config that I use in all my projects).

[HiDeoo]

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.

All the tasks from my previous message are completed, all the remaining errors are fixed and the last remaning task is waiting for the next major version of typescript-eslint which should land very soon. When available, I'll do a last update to the branch and push all the changes.