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

Use eslint v9 flat config #699

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

Conversation

plbstl
Copy link

@plbstl plbstl commented Sep 3, 2024

Priority*

  • High: This PR needs to be merged first for other tasks.
  • Middle: This PR should be merged quickly to prevent conflicts due to common changes. (default)
  • Low: This PR does not affect other tasks, so it can be merged later.

Purpose of the PR*

Use eslint version 9 flat config.

Changes*

This PR upgrades eslint to version 9 and uses the flat config.

How to check the feature

Add code that warns or errors when eslint is set up correctly.

Reference

@plbstl plbstl requested a review from Jonghakseo as a code owner September 3, 2024 19:06
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. We will check and reply to you as soon as possible.

eslint.config.mjs Outdated Show resolved Hide resolved
eslint.config.mjs Outdated Show resolved Hide resolved
@PatrykKuniczak
Copy link
Collaborator

Let's remove also:

// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore

from packages/i18n and

// eslint-disable-next-line
// @ts-ignore

from pages/content-runtime/lib

and check all // eslint-disable-next-line e.g. for refresh and reload because the most of it isn't necessary probably 😄

@plbstl
Copy link
Author

plbstl commented Sep 5, 2024

Okay, I'll make those changes now

@PatrykKuniczak
Copy link
Collaborator

@plbstl Let's rebase ASAP, you'll avoid more conflicts, because i'm working on other new things 😄

@plbstl
Copy link
Author

plbstl commented Sep 5, 2024

For the recent changes in #697, I was thinking pnpm exec may be better than pnpx (pnpm dlx) since it uses the locally installed packages.

https://pnpm.io/cli/exec#examples

package.json Outdated Show resolved Hide resolved
@PatrykKuniczak
Copy link
Collaborator

@plbstl Are you able to set this --flag unstable_ts_config inside config file, instead of the flag in each script?

@plbstl
Copy link
Author

plbstl commented Sep 6, 2024

I checked the source code, the option is only available as a CLI flag

@plbstl
Copy link
Author

plbstl commented Sep 6, 2024

Replaced eslint-plugin-import with eslint-plugin-import-x as it supports eslint v9.

It also has an additional rule no-rename-default under Helpful warnings section.

@PatrykKuniczak PatrykKuniczak added the enhancement New feature or request label Sep 7, 2024
@PatrykKuniczak
Copy link
Collaborator

@plbstl You've merge main wrongly, because i see unnecessary files(which shouldn't be shown as change here) in Files changed

And you have a lot of conflicts, let's solve it

@plbstl
Copy link
Author

plbstl commented Sep 8, 2024

@PatrykKuniczak The conflicts have now been resolved

@PatrykKuniczak PatrykKuniczak mentioned this pull request Sep 9, 2024
3 tasks
@PatrykKuniczak
Copy link
Collaborator

PatrykKuniczak commented Sep 9, 2024

It doesn't work at all
image

First of all, eslint config wasn't found be IDE
Second thing, when i've added
tsconfig for this eslint:

{
  "$schema": "https://json.schemastore.org/tsconfig",
  "display": "Chrome Extension Utils",
  "extends": "./packages/tsconfig/base.json",
  "compilerOptions": {
    "noEmit": false
  },
  "include": ["eslint.config.ts"]
}

and "ready": "tsc -p eslint.tsconfig.json && turbo ready" for package.json and i've run it.
But it still doesn't work because of the lack of types.

And after all we have this warns, which are the problem, why we not decide to bump eslint to version 9:
image

I think this PR is overkill for that simply solution as eslint is.
IDN what to do with it, maybe @Jonghakseo have sth to say about it.
IMO the best idea is wait a little, when this libs start to support eslint 9

PS: After bundle this eslint.config.js file still isn't detected, maybe change this .ts to .js hymm maybe let's try to do workaround for this.

@plbstl
Copy link
Author

plbstl commented Sep 10, 2024

Can you tell me the name of the IDE? The repo does not contain any config files for IDEs. You have to instruct the specific IDE eslint plugin to load the config. Just as discussed in #699 (comment)

@typescript-eslint/eslint-plugin and @typescript-eslint/parser are no longer needed, a mistake on my part for not removing the dependencies. See https://typescript-eslint.io/packages/typescript-eslint/#migrating-from-legacy-config-setups

Same goes for eslint-plugin-jsx-a11y. Updating https://github.com/jsx-eslint/eslint-plugin-jsx-a11y to 6.10 fixes the peer dependency issue, as it nows supports eslint version 9 and flat config.

I don't know where the eslint-plugin-import issue is coming from. eslint-import-resolver-typescript has correct peer dependencies (https://github.com/import-js/eslint-import-resolver-typescript/blob/01902942c42292ab3cd266c2d6855c76a2ab5d7e/package.json#L64) and eslint-plugin-import-x is used and it is not depending on eslint-plugin-import.

Since eslint-config-airbnb-typescript is archived (https://github.com/iamturns/eslint-config-airbnb-typescript), I don't think it'll ever support eslint version 9, flat config or even update its peer dependencies.

@PatrykKuniczak
Copy link
Collaborator

Can you tell me the name of the IDE? The repo does not contain any config files for IDEs. You have to instruct the specific IDE eslint plugin to load the config. Just as discussed in #699

@typescript-eslint/eslint-plugin and @typescript-eslint/parser are no longer needed, a mistake on my part for not removing the dependencies. See https://typescript-eslint.io/packages/typescript-eslint/#migrating-from-legacy-config-setups

Same goes for eslint-plugin-jsx-a11y. Updating eslint-plugin-jsx-a11y to 6.10 fixes the peer dependency issue.

I don't know where the eslint-plugin-import issue is coming from. eslint-import-resolver-typescript has correct peer dependencies (https://github.com/import-js/eslint-import-resolver-typescript/blob/01902942c42292ab3cd266c2d6855c76a2ab5d7e/package.json#L64) and eslint-plugin-import-x is used and it is not depending on eslint-plugin-import.

Since eslint-config-airbnb-typescript is archived (https://github.com/iamturns/eslint-config-airbnb-typescript), I don't think it'll ever support version 9 or even update its peer dependencies.

You've pinned This PR in Just as discussed in https://github.com/Jonghakseo/chrome-extension-boilerplate-react-vite/pull/699 it's correct?

I have been using WebStorm

@plbstl
Copy link
Author

plbstl commented Sep 10, 2024

You've pinned This PR in Just as discussed in https://github.com/Jonghakseo/chrome-extension-boilerplate-react-vite/pull/699 it's correct?

This is what I meant to link to: Just as discussed in https://github.com/Jonghakseo/chrome-extension-boilerplate-react-vite/pull/699#discussion_r1747710475

@PatrykKuniczak
Copy link
Collaborator

@plbstl In the future, you can Copy link
image

It's easier to navigate by everybody, because it creates clickable link 😄

@PatrykKuniczak
Copy link
Collaborator

@plbstl Let's rebase branch

@plbstl
Copy link
Author

plbstl commented Sep 10, 2024

@plbstl In the future, you can Copy link image

It's easier to navigate by everybody, because it creates clickable link 😄

Thank you, that's what I did but for some reason it added a search param for a notification_referrer_id before the URL hash

https://github.com/Jonghakseo/chrome-extension-boilerplate-react-vite/pull/699?notification_referrer_id=id#discussion_r1747710475

@PatrykKuniczak PatrykKuniczak linked an issue Sep 11, 2024 that may be closed by this pull request
Copy link
Collaborator

@PatrykKuniczak PatrykKuniczak left a comment

Choose a reason for hiding this comment

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

All your imports:
import * as anything from "anything"

should be:
import anything from "anything"

If it doesn't work you don't include or set types in tsconfig properly

@plbstl
Copy link
Author

plbstl commented Sep 11, 2024

All your imports: import * as anything from "anything"

should be: import anything from "anything"

If it doesn't work you don't include or set types in tsconfig properly

It works. Its just that eslint shows a warning for the import-x/no-named-as-default-member rule.

Should I change it back and turn off the rule? Or change the imports to named imports? Or something else?

@PatrykKuniczak
Copy link
Collaborator

All your imports: import * as anything from "anything"
should be: import anything from "anything"
If it doesn't work you don't include or set types in tsconfig properly

It works. Its just that eslint shows a warning for the import-x/no-named-as-default-member rule.

Should I change it back and turn off the rule? Or change the imports to named imports? Or something else?

Maybe set it off, because we're using default imports

@plbstl
Copy link
Author

plbstl commented Sep 11, 2024

The change works for import * as ts from 'typescript-eslint'. However, the remaining import * as anything from "anything" statements are for esbuild imports which shows eslint error:

Screenshot 2024-09-11 at 15 33 09

Going through the esbuild docs, the examples I see uses a import * as esbuild from 'esbuild' import.

@PatrykKuniczak
Copy link
Collaborator

PatrykKuniczak commented Sep 11, 2024

@plbstl You're right, because no default is exported from there :)
image

Leave it, in the way you've done it

But there's greater problem. my IDE still can't find config, because of .ts i think this should be .js because it works.

Or use .ts but build this file on postinstall and IDE will be using bundled version of this, let's check if it will be working 😄

If yes, then add .js to .gitignore and it will work perfect :)

PS: With second approach, we can have 100% ts coverage, maybe it's worth efford 😄

plbstl and others added 25 commits September 15, 2024 10:37
eslint-vscode does not show issues
type=module is present in package.json
prettier's default `--ignore-path` is `[.gitignore, .prettierignore]`
Copy link
Collaborator

@PatrykKuniczak PatrykKuniczak left a comment

Choose a reason for hiding this comment

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

I've resolve conflicts, and add 1 missing types, but theres still some work:
As you can see, there's a bunch of warns:
image

Let's fix it, but:
https://github.com/iamturns/eslint-config-airbnb-typescript
Isn't longer supported, you need to find sth else, which supporting eslint v9
eslint-config-airbnb-base is also have last update 3 years ago
eslint-plugin-import is still as a peer deps
And there's missing types:
image

Let's try to find solution, because we don't want to merge it, becasue of potential problems with deps, we want to avoid it, that's why we don't decided to implement it for now, but i hope you're able to fix it.

PS: For reproduce that error try e.g. pnpm add -D nodemon -w

@PatrykKuniczak
Copy link
Collaborator

@plbstl Are you working on it?

@PatrykKuniczak
Copy link
Collaborator

@plbstl Are you eager to go on?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn about deprecated packages Use eslint 9
2 participants