Jonghakseo / chrome-extension-boilerplate-react-vite

Chrome Extension Boilerplate with React + Vite + Typescript
MIT License
2.26k stars 329 forks source link

Use eslint v9 flat config #699

Open plbstl opened 3 weeks ago

plbstl commented 3 weeks ago

Priority*

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

PatrykKuniczak commented 3 weeks ago

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 commented 3 weeks ago

Okay, I'll make those changes now

PatrykKuniczak commented 3 weeks ago

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

plbstl commented 3 weeks ago

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

PatrykKuniczak commented 3 weeks ago

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

plbstl commented 3 weeks ago

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

plbstl commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 2 weeks ago

@PatrykKuniczak The conflicts have now been resolved

PatrykKuniczak commented 2 weeks ago

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 commented 2 weeks ago

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 https://github.com/Jonghakseo/chrome-extension-boilerplate-react-vite/pull/699#discussion_r1747710475

@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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

@plbstl In the future, you can Copy link image

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

PatrykKuniczak commented 2 weeks ago

@plbstl Let's rebase branch

plbstl commented 2 weeks ago

@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
plbstl commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

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 😄

The latest changes have done exactly that, and it works well.

PatrykKuniczak commented 2 weeks ago

@plbstl I'll check it tomorrow morning 😸

PatrykKuniczak commented 2 weeks ago

@plbstl Rebase and resolve conflicts

plbstl commented 2 weeks ago

I have merged in the main branch, but I did not run into merge conflicts

PatrykKuniczak commented 2 weeks ago

@plbstl I have errors still: image

Because this packages doesn't export default nothing: image

Let's change it to: import * as name from 'name';

It's working :)

plbstl commented 2 weeks ago

@plbstl

I have errors still:

image

Because this packages doesn't export default nothing:

image

Let's change it to:

import * as name from 'name';

It's working :)

I think it's because the root tsconfig was removed. It has to be added back since it includes configuration for eslint.config.ts

VSCode does not error for that, it made me totally forget about it

The root tsconfig wasn't only for the build script

PatrykKuniczak commented 2 weeks ago

@plbstl Yeah, the tsconfig are using also for this, but anyway i'm thinking if it's good, but yeah, do it like you said, but extends base.json

plbstl commented 2 weeks ago

I have made the relevant changes

PatrykKuniczak commented 2 weeks ago

@plbstl image Why? xD

Let's rebase locally and see those conflicts, if there's no conflicts, then IDN xD

plbstl commented 2 weeks ago

image

You did sth wrong for sure, let's rebase it and i'll try it again :)

It can be changed that pnpm build is run first. Or add @ts-ignore back.

PatrykKuniczak commented 2 weeks ago

OOOKKK, that's what i'm talking about with @Jonghakseo I'll open PR, to solve this

PatrykKuniczak commented 2 weeks ago

It'll be fixed in #740 After this, everything should be OK. Thanks again for your contribution <3

PatrykKuniczak commented 2 weeks ago

@plbstl If you'll eager to contribute a little more visit our Discord and i can explain you more 😄

plbstl commented 2 weeks ago

@PatrykKuniczak yeah sure. I just joined the server 🚀

PatrykKuniczak commented 1 week ago

@plbstl Are you working on it?