vite icon indicating copy to clipboard operation
vite copied to clipboard

feat(create-vite): add eslint.config.js to React templates

Open smeng9 opened this issue 2 years ago • 7 comments

Description

Hi @ArnaudBarre As a follow up on https://github.com/vitejs/vite/pull/12801 I have helped to create an example using the latest eslint.config.js format of config

Additional context


What is the purpose of this pull request?

  • [ ] Bug fix
  • [X] New Feature
  • [ ] Documentation update
  • [ ] Other

Before submitting the PR, please make sure you do the following

  • [X] Read the Contributing Guidelines.
  • [X] Read the Pull Request Guidelines and follow the PR Title Convention.
  • [X] Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • [X] Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • [X] Ideally, include relevant tests that fail without this PR but pass with it.

smeng9 avatar Apr 14 '23 08:04 smeng9

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Thanks for this. I tested it and work as expected. I'm really surprised that we require to install to more packages for this, which is not mentioned in the doc that prefer to explain me what is object shorthand 🤦‍♂️

The documentation on this is so poor that I'm not sure we should push new Vite users that could be people still learning web basics into this. I will see what the react of team think of that. Thanks for the work, in any case this will be useful and merge when this become the official config format

ArnaudBarre avatar Apr 14 '23 12:04 ArnaudBarre

Hi! We decided to wait for eslint to switch the default config format to the new one before adding it to the template. The PR can be kept open and updated once the announcement is made (and hopefully there is more documentation)

ArnaudBarre avatar Apr 20 '23 08:04 ArnaudBarre

The default switched to flat config with today's 9.0.0 release: https://eslint.org/blog/2024/04/eslint-v9.0.0-released/#flat-config-is-now-the-default-and-has-some-changes

cherewaty avatar Apr 05 '24 23:04 cherewaty

We still need to wait https://github.com/jsx-eslint/eslint-plugin-react/issues/3699 and https://github.com/facebook/react/issues/28313 to be resolved

smeng9 avatar Apr 06 '24 03:04 smeng9

Hi @ArnaudBarre Shall we consider using the @eslint/compat here https://eslint.org/blog/2024/05/eslint-compatibility-utilities/ before the dependent eslint plugin repo release a fix for the incompatible api?

smeng9 avatar May 23 '24 03:05 smeng9

Yeah I think this is better for new users to get started with the new config at the cost of one more compat package. Each usage of the compat should prefix with a comment // Remove compat when the plugin is compatible with ESLint v9: https://github.com/..

But first we should wait for the VS Code extension to support v9 without pre-release: https://github.com/Microsoft/vscode-eslint/blob/HEAD/CHANGELOG.md

ArnaudBarre avatar May 23 '24 08:05 ArnaudBarre

Thanks for the update, the only downside is that we can peer deps warning

Edit: it seems they aye still conflicts, I test locally once resolved

ArnaudBarre avatar Jun 18 '24 11:06 ArnaudBarre

@ArnaudBarre All issues should be fixed now

smeng9 avatar Jun 18 '24 12:06 smeng9

When I run npm install, I get:

bjorn@Bjorns-MacBook-Pro vite-project % npm i
npm error code ERESOLVE
npm error ERESOLVE unable to resolve dependency tree
npm error
npm error While resolving: [email protected]
npm error Found: [email protected]
npm error node_modules/eslint
npm error   dev eslint@"^9.5.0" from the root project
npm error
npm error Could not resolve dependency:
npm error peer eslint@"^8.56.0" from @typescript-eslint/[email protected]
npm error node_modules/@typescript-eslint/parser
npm error   dev @typescript-eslint/parser@"^7.13.1" from the root project
npm error   peer @typescript-eslint/parser@"^7.0.0" from @typescript-eslint/[email protected]
npm error   node_modules/@typescript-eslint/eslint-plugin
npm error     dev @typescript-eslint/eslint-plugin@"^7.13.1" from the root project
npm error
npm error Fix the upstream dependency conflict, or retry
npm error this command with --force or --legacy-peer-deps
npm error to accept an incorrect (and potentially broken) dependency resolution.

Do we want to use the alpha versions of @typescript-eslint/* to prevent this? (Could move to typescript-eslint along the way too)

bluwy avatar Jun 19 '24 07:06 bluwy

I didn't know npm was that strict. I only got warning with bun/yarn. We will get peer deps issues for react plugins too, so I don't know what we can do

ArnaudBarre avatar Jun 19 '24 07:06 ArnaudBarre

Ah yeah for the JS template, I get:

npm error Could not resolve dependency:
npm error peer eslint@"^3 || ^4 || ^5 || ^6 || ^7 || ^8" from [email protected]
npm error node_modules/eslint-plugin-react
npm error   dev eslint-plugin-react@"^7.34.2" from the root project

Since the TS template, doesn't have eslint-plugin-react, maybe we can remove it for the JS template too? (unless I forgot what's its use for).

bluwy avatar Jun 19 '24 07:06 bluwy

I only tested using yarn. Shall we do

"overrides": {
    "@typescript-eslint/parser": {
        "eslint": "^9.5.0"
    }
   ...
}

in package.json ?

smeng9 avatar Jun 19 '24 07:06 smeng9

After some research we can probably add the following field to package.json

"overrides": {
    "eslint": "$eslint"
}

smeng9 avatar Jun 19 '24 07:06 smeng9

Hi @ArnaudBarre is there anything else we should do in this merge request to move the ecosystem forward?

smeng9 avatar Jul 03 '24 15:07 smeng9

Just some helpful info:

  • @typescript-eslint/parser and @typescript-eslint/eslint-plugin packages are not needed when using typescript-eslint. See https://typescript-eslint.io/packages/typescript-eslint

  • eslint-plugin-react now has a merged PR for eslint v9 support. https://github.com/jsx-eslint/eslint-plugin-react/pull/3759

  • eslint-plugin-react-hooks@canary supports v9. https://github.com/facebook/react/pull/28773

plbstl avatar Jul 16 '24 20:07 plbstl

Here is a working config.

The caveat is that npm install errors because of a dependency conflict. The --force flag works fixes it. yarn and pnpm shows a warning.

import js from '@eslint/js'
import reactHooks from 'eslint-plugin-react-hooks'
import reactRefresh from 'eslint-plugin-react-refresh'
import reactJsxRuntime from 'eslint-plugin-react/configs/jsx-runtime.js'
import reactRecommended from 'eslint-plugin-react/configs/recommended.js'
import globals from 'globals'
import ts from 'typescript-eslint'

export default ts.config(
  // ignore patterns (.eslintignore)
  { ignores: ['dist', 'eslint.config.js'] },

  // misc
  { languageOptions: { globals: { ...globals.browser, ...globals.es2020 } } },

  // typescript eslint
  js.configs.recommended,
  ...ts.configs.recommended,
  {
    languageOptions: {
      parserOptions: {
    	ecmaVersion: 'latest',
	    sourceType: 'module',
        project: ['./tsconfig.json', 'tsconfig.app.json', './tsconfig.node.json'],
        tsconfigRootDir: import.meta.dirname
      }
    }
  },

  // react
  reactRecommended,
  reactJsxRuntime,
  { settings: { react: { version: 'detect' } } },

  // react hooks
  {
    plugins: { 'react-hooks': reactHooks },
    rules: { ...reactHooks.configs.recommended.rules }
  },

  // react refresh
  {
    plugins: { 'react-refresh': reactRefresh },
    rules: {
      'react-refresh/only-export-components': ['warn', { allowConstantExport: true }]
    }
  },

  // linter
  { linterOptions: { reportUnusedDisableDirectives: 'error' } }
)
{
    "@eslint/js": "9.7.0",
    "@types/eslint__js": "8.42.3",
    "@types/node": "20.14.11",
    "@types/react": "18.3.3",
    "@types/react-dom": "18.3.0",
    "@vitejs/plugin-react-swc": "3.7.0",
    "eslint": "9.7.0",
    "eslint-plugin-react": "7.34.4",
    "eslint-plugin-react-hooks": "4.6.2", // not using canary
    "eslint-plugin-react-refresh": "0.4.8",
    "globals": "15.8.0",
    "typescript": "5.5.3",
    "typescript-eslint": "7.16.1",
    "vite": "5.3.4"
  }

plbstl avatar Jul 17 '24 14:07 plbstl

@ArnaudBarre Sounds good to take over this PR

smeng9 avatar Jul 23 '24 23:07 smeng9