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(create-vite): add eslint.config.js to React templates #12860

Merged
merged 17 commits into from
Jul 31, 2024
Merged

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

merged 17 commits into from
Jul 31, 2024

Conversation

smeng9
Copy link
Contributor

@smeng9 smeng9 commented Apr 14, 2023

Description

Hi @ArnaudBarre As a follow up on #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
  • New Feature
  • Documentation update
  • Other

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

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

@stackblitz
Copy link

stackblitz bot commented Apr 14, 2023

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

@ArnaudBarre
Copy link
Member

ArnaudBarre commented Apr 14, 2023

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
Copy link
Member

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)

@bluwy bluwy added p2-nice-to-have Not breaking anything but nice to have (priority) on hold labels Sep 19, 2023
@cherewaty
Copy link

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

@smeng9
Copy link
Contributor Author

smeng9 commented Apr 6, 2024

We still need to wait jsx-eslint/eslint-plugin-react#3699 and facebook/react#28313 to be resolved

@smeng9
Copy link
Contributor Author

smeng9 commented May 23, 2024

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?

@ArnaudBarre
Copy link
Member

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
Copy link
Member

ArnaudBarre commented Jun 18, 2024

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

@smeng9
Copy link
Contributor Author

smeng9 commented Jun 18, 2024

@ArnaudBarre All issues should be fixed now

ArnaudBarre
ArnaudBarre previously approved these changes Jun 18, 2024
@bluwy bluwy removed the on hold label Jun 19, 2024
@bluwy
Copy link
Member

bluwy commented Jun 19, 2024

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: vite-project@0.0.0
npm error Found: eslint@9.5.0
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/parser@7.13.1
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/eslint-plugin@7.13.1
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)

@ArnaudBarre
Copy link
Member

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

@bluwy
Copy link
Member

bluwy commented Jun 19, 2024

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 eslint-plugin-react@7.34.3
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).

@smeng9
Copy link
Contributor Author

smeng9 commented Jun 19, 2024

I only tested using yarn. Shall we do

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

in package.json ?

@smeng9
Copy link
Contributor Author

smeng9 commented Jun 19, 2024

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

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

@smeng9
Copy link
Contributor Author

smeng9 commented Jul 3, 2024

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

@plbstl
Copy link

plbstl commented Jul 16, 2024

Just some helpful info:

@plbstl
Copy link

plbstl commented Jul 17, 2024

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"
  }

Copy link
Member

@ArnaudBarre ArnaudBarre left a comment

Choose a reason for hiding this comment

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

After discussion with @bluwy we think it's better to switch to full v9 compatibility, remove the compat layer and use the canary version of react hooks plugin for now.

Also using the new typescript-eslint with the typed config would be good for the TS package: https://typescript-eslint.io/packages/typescript-eslint/#advanced-usage

Edit: Sorry for the long answer, we can take over the PR if don't have time to update it in the next days, no problem for us, thanks a lot for the initial work and exploring the compat option!

@smeng9
Copy link
Contributor Author

smeng9 commented Jul 23, 2024

@ArnaudBarre Sounds good to take over this PR

@bluwy bluwy requested a review from ArnaudBarre July 30, 2024 06:19
Copy link
Member

@ArnaudBarre ArnaudBarre left a comment

Choose a reason for hiding this comment

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

Sorry totally forgot about it!

bluwy
bluwy previously approved these changes Jul 30, 2024
ArnaudBarre
ArnaudBarre previously approved these changes Jul 30, 2024
@ArnaudBarre ArnaudBarre self-requested a review July 30, 2024 16:53
@ArnaudBarre ArnaudBarre dismissed stale reviews from bluwy and themself via 9f6a689 July 30, 2024 17:32
@bluwy bluwy merged commit dcfa75c into vitejs:main Jul 31, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants