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

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

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: [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)

@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 [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).

@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?

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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants