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: use typescript-eslint@v6's reworked configs #7425

Merged
merged 14 commits into from
Jul 3, 2023

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Jun 20, 2023

Changes

Posting this as a reference for https://discord.com/channels/830184174198718474/845430950191038464/1120563248782114986:

👋 I'm working on testing out reworked preset configs for the next major version of typescript-eslint: typescript-eslint/typescript-eslint#6760. I'd like to try them out on Astro because it's one of my favorite open source projects ❤️. Should I file an issue for this? (I could only find the bug template).
Benefits of trying this out means I'll help out with the ESLint config in withastro/astro & listen even more closely & seriously to your feedback on the new preset configs!

Edit: the response there was positive, so un-drafting! Thanks!

Upgraded typescript-eslint to v6, with reworked ESLint configurations.

You can read https://typescript-eslint.io/blog/announcing-typescript-eslint-v6-beta#user-facing-breaking-changes for the rationale behind the config changes. Essentially, the new recommended starter configs are:

  • "plugin:@typescript-eslint/recommended-type-checked"
  • "plugin:@typescript-eslint/stylistic-type-checked"

I've commented any changes I suspect you might disagree with inline, for discussion. Would love to know what you all think!

@changeset-bot
Copy link

changeset-bot bot commented Jun 20, 2023

⚠️ No Changeset found

Latest commit: b712a2f

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.

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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: create-astro Related to the `create-astro` package (scope) pkg: lit Related to Lit (scope) pkg: preact Related to Preact (scope) pkg: react Related to React (scope) pkg: solid Related to Solid (scope) pkg: vue Related to Vue (scope) pkg: integration Related to any renderer integration (scope) feat: markdown Related to Markdown (scope) 🚨 action Modifies GitHub Actions labels Jun 20, 2023
.eslintrc.cjs Outdated Show resolved Hide resolved
@@ -28,7 +28,7 @@
"test:e2e": "cd packages/astro && pnpm playwright install && pnpm run test:e2e",
"test:e2e:match": "cd packages/astro && pnpm playwright install && pnpm run test:e2e:match",
"benchmark": "astro-benchmark",
"lint": "eslint --cache .",
"lint": "eslint . --report-unused-disable-directives",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not strictly necessary, but I've found this to be a nice cleanup.

Copy link
Member

Choose a reason for hiding this comment

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

I think we'd want to still keep the --cache too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ehh I've found the cache to be ... unreliable when working with type checking. Theoretically a file's lint result could be impacted by a couple of other factors -other files it imports types from, TSConfig settings- so caching becomes difficult.

Copy link
Member

Choose a reason for hiding this comment

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

I won't deny that I've seen --cache acting strange before, so I could be persuaded removing it for now if there's no rejections from others.

@@ -160,9 +160,8 @@ function prefixError(err: any, prefix: string) {
const wrappedError = new Error(`${prefix}${err ? `: ${err}` : ''}`);
try {
wrappedError.stack = err.stack;
// @ts-expect-error
wrappedError.cause = err;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added target: ES2022 in the tsconfig.json so this line wouldn't need a // @ts-expect-error.

packages/telemetry/src/system-info.ts Outdated Show resolved Hide resolved
packages/integrations/image/src/utils/workerPool.ts Outdated Show resolved Hide resolved
packages/integrations/image/src/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

I noticed that some changes use the bang operator !. I consider this very unsafe, and I wonder if we should change our code there or at least leave some comments explaining why it's safe. cc @withastro/maintainers-core

packages/astro/e2e/hydration-race.test.js Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg changed the title [Reference] feat: use typescript-eslint@v6's reworked configs feat: use typescript-eslint@v6's reworked configs Jun 20, 2023
@JoshuaKGoldberg
Copy link
Contributor Author

CI / Lint (pull_request) Cancelled after 3m

...works on my machine! (as in, pnpm lint after pnpm build produces no errors. Undrafting 🤷

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review July 1, 2023 21:45
@@ -1,23 +1,53 @@
module.exports = {
extends: [
Copy link
Member

Choose a reason for hiding this comment

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

I wonder, since this is a breaking change, should we try to move to use the new eslint configuration format? https://eslint.org/docs/latest/use/configure/configuration-files-new

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 action Modifies GitHub Actions feat: markdown Related to Markdown (scope) pkg: astro Related to the core `astro` package (scope) pkg: create-astro Related to the `create-astro` package (scope) pkg: integration Related to any renderer integration (scope) pkg: lit Related to Lit (scope) pkg: preact Related to Preact (scope) pkg: react Related to React (scope) pkg: solid Related to Solid (scope) pkg: vue Related to Vue (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants