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

Next should not require that eslint-config-next be installed separately #26348

Closed
aslilac opened this issue Jun 19, 2021 · 9 comments · Fixed by #26584
Closed

Next should not require that eslint-config-next be installed separately #26348

aslilac opened this issue Jun 19, 2021 · 9 comments · Fixed by #26584
Assignees
Milestone

Comments

@aslilac
Copy link

aslilac commented Jun 19, 2021

What version of Next.js are you using?

11.0.0

What version of Node.js are you using?

14.16.0

What browser are you using?

Chrome

What operating system are you using?

macOS

How are you deploying your application?

Vercel

Describe the Bug

next build fails when eslint-config-next isn't installed

Expected Behavior

If Next wants to require eslint-config-next be installed to successfully build a project then it should be listed as a dependency of Next itself. If eslint-config-next is not installed Next should still be able to successfully build a project, and it should not require that you manually specify eslint.ignoreDuringBuilds in your next.config.js.

It's incredibly confusing that Next will now always fail to build in it's default configuration, where this has never been an issue in any previous versions.

To Reproduce

Upgrade to Next 11 without installing eslint-config-next

@aslilac aslilac added the bug Issue was opened via the bug report template. label Jun 19, 2021
@mikestopcontinues
Copy link

This issue goes a few steps deeper.

1. Error occurs even when eslint-plugin-next is used by eslint config

First thing I did upon hearing about the new plugin was add it to my current eslint config, which I use across all my next apps. (For reference: @sitearcade/eslint-config). But against all logic, next still insists on installing eslint-config-next.

So I installed config-next, but...

2. Eslint plugins not found, even when present

When deploying (to Vercel), I'm getting errors that all of my eslint plugins are missing, even though they're definitely being installed. My config (@sitearcade/eslint-config) lists them all as peerDeps, so they're right where eslint expects to find them. And deploys prior to Next 11 worked totally fine.

3. No clear path forward...

eslint-config-next is useful, and I do see how for users who hadn't been using eslint before, it helps a good deal. But because of the overrides it defines, extending it is not particularly easy.

I agree with @partheseas that the new eslint functionality should either be opt-in OR fallback behavior if a user isn't already using an eslint config. I also think there should be some more thought given to how to properly integrate with the eslint ecosystem and then expand the docs to explain. Thanks!

@housseindjirdeh housseindjirdeh self-assigned this Jun 21, 2021
@housseindjirdeh
Copy link
Collaborator

housseindjirdeh commented Jun 22, 2021

This is great feedback, thank you both. I can see how restrictive it can feel to have to install eslint-config-next, especially if you don't ever intend to use it. I was already planning to update things to check if either eslint-plugin-next or eslint-config-next is installed, but I think a few more changes will also be necessary.

To summarize my thinking:

  1. If users run next lint and no ESLint configuration is detected (first time setup), they'll be asked which default configuration they would like to start with:

    1. Next.js base configuration along with the stricter Core Web Vitals rule set: extends: ['next', 'next/core-web-vitals']
    2. Next.js base configuration: extends: 'next'
    3. No default configuration (user will take care of setting up ESLint themselves)

    If option i or ii is selected, both eslint and eslint-config-next will be installed automatically.

  2. If users run next lint and a ESLint configuration is present, the linter will run normally and any warnings and errors will be displayed to the user.

  3. If eslint.ignoreDuringBuilds is not enabled and a user runs next build with no ESLint setup whatsoever - only the eslint library will be a requirement to be installed (and the user will either have to run next lint to set things up or install it themselves separately). Neither eslint-config-next or eslint-plugin-next will be required to be installed.

  4. During both next build and next lint, a warning message will display that recommend installing and using eslint-plugin-next or eslint-config-next if none is detected in an already existing ESLint configuration. This will only be a warning and builds will still pass successfully.

  5. Docs will be updated to explain all the above along with additional detail

Point 3 is most relevant to the original comment in this thread, but I wanted to summarize how I think the entire feature can be improved since it's kind of relevant. I think this will remove the burden of needing to install eslint-config-next plus improve the developer experience quite a bit. Please let me know what you think, and any suggestions are welcome!

@aslilac
Copy link
Author

aslilac commented Jun 22, 2021

Related to point 3, I think that should also be a warning rather than a requirement. Check if "eslint" is installed, and if it is run linting, if it isn't print a warning

The rest of it all sounds perfectly reasonable :)

@mikestopcontinues
Copy link

@housseindjirdeh This sounds great. Point 2 will fix my issue, and I agree with @partheseas on using a warning in point 3. Thanks for your help!

@timneutkens timneutkens added kind: bug and removed bug Issue was opened via the bug report template. labels Jun 22, 2021
@timneutkens timneutkens added this to the Iteration 22 milestone Jun 22, 2021
@tw0517tw
Copy link

I've encountered some lint issues with monorepo setup and had some discussion in #26182 , maybe it's worth it to also add the part with monorepo developers?

@housseindjirdeh
Copy link
Collaborator

Opened #26584 to address all the points in my previous comment and improve the overall developer experience.

Related to point 3, I think that should also be a warning rather than a requirement.

I agree with @partheseas on using a warning in point 3.

Thanks folks, and noted. I've gone ahead with just displaying a message instead of requiring the user to install any package.

Screen Shot 2021-06-25 at 4 15 47 PM

You might see the word error (subject to change) but it'll act like any warning and will not ever fail the build :)

@housseindjirdeh
Copy link
Collaborator

@tw0517tw Are there any specifics you think can be improved with regards to monorepos? I would love to hear your thoughts on the new developer experience, but otherwise we can definitely take this discussion into more detail in #26182 :)

@kodiakhq kodiakhq bot closed this as completed in #26584 Aug 4, 2021
kodiakhq bot pushed a commit that referenced this issue Aug 4, 2021
…e first time (#26584)

This PR introduces an improved developer experience when `next lint` is run for the first time.

### Current behavior

`eslint-config-next` is a required package that must be installed before proceeding with `next lint` or `next build`:

![image](https://user-images.githubusercontent.com/12476932/123468791-43088100-d5c0-11eb-9ad0-5beb80b6c968.png)

Although this has helped many developers start using the new ESLint config, this has also resulted in a few issues:

- Users are required to install the full config (`eslint-config-next`) even if they do not use it or use the Next.js plugin directly (`eslint-plugin-next`).
  - #26348

- There's some confusion  on why `eslint-config-next` needs to be installed or how it should be used instead of `eslint-plugin-next`.
  - #26574
  - #26475
  - #26438

### New behavior

Instead of enforcing `eslint-config-next` as a required package, this PR prompts the user by asking what config they would like to start. This happens when `next lint` is run for the first time **and** if no ESLint configuration is detected in the application.

<img src="https://user-images.githubusercontent.com/12476932/124331177-e1668a80-db5c-11eb-8915-38d3dc20f5d4.gif" width="800" />

- The CLI will take care of installing `eslint` or `eslint-config-next` if either is not already installed
- Users now have the option to choose between a strict configuration (`next/core-web-vitals`) or just the base configuration (`next`)
- For users that decide to create their own ESLint configuration, or already have an existing one, **installing `eslint-config-next` will not be a requirement for `next lint` or `next build` to run**. A warning message will just show if the Next.js ESLint plugin is not detected in an ESLint config. 

  <img width="682" alt="Screen Shot 2021-06-25 at 3 02 12 PM" src="https://user-images.githubusercontent.com/12476932/123473329-6cc4a680-d5c6-11eb-9a57-d5c0b89a2732.png">

---

In addition, this PR also:

- Fixes #26348
- Updates documentation to make it more clear what approach to take for new and existing ESLint configurations
@stefanprobst
Copy link
Contributor

for me, installing eslint-config-next is still required, because of this check where path.dirname throws a TypeError when eslint-config-next does not resolve - is this expected?

flybayer pushed a commit to blitz-js/next.js that referenced this issue Aug 19, 2021
…e first time (vercel#26584)

This PR introduces an improved developer experience when `next lint` is run for the first time.

### Current behavior

`eslint-config-next` is a required package that must be installed before proceeding with `next lint` or `next build`:

![image](https://user-images.githubusercontent.com/12476932/123468791-43088100-d5c0-11eb-9ad0-5beb80b6c968.png)

Although this has helped many developers start using the new ESLint config, this has also resulted in a few issues:

- Users are required to install the full config (`eslint-config-next`) even if they do not use it or use the Next.js plugin directly (`eslint-plugin-next`).
  - vercel#26348

- There's some confusion  on why `eslint-config-next` needs to be installed or how it should be used instead of `eslint-plugin-next`.
  - vercel#26574
  - vercel#26475
  - vercel#26438

### New behavior

Instead of enforcing `eslint-config-next` as a required package, this PR prompts the user by asking what config they would like to start. This happens when `next lint` is run for the first time **and** if no ESLint configuration is detected in the application.

<img src="https://user-images.githubusercontent.com/12476932/124331177-e1668a80-db5c-11eb-8915-38d3dc20f5d4.gif" width="800" />

- The CLI will take care of installing `eslint` or `eslint-config-next` if either is not already installed
- Users now have the option to choose between a strict configuration (`next/core-web-vitals`) or just the base configuration (`next`)
- For users that decide to create their own ESLint configuration, or already have an existing one, **installing `eslint-config-next` will not be a requirement for `next lint` or `next build` to run**. A warning message will just show if the Next.js ESLint plugin is not detected in an ESLint config. 

  <img width="682" alt="Screen Shot 2021-06-25 at 3 02 12 PM" src="https://user-images.githubusercontent.com/12476932/123473329-6cc4a680-d5c6-11eb-9a57-d5c0b89a2732.png">

---

In addition, this PR also:

- Fixes vercel#26348
- Updates documentation to make it more clear what approach to take for new and existing ESLint configurations
@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants