[ESLint] Introduce a new setup process when `next lint` is run for the first time #26584

housseindjirdeh · 2021-06-24T21:38:02Z

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:

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).
- Next should not require that eslint-config-next be installed separately #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.

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.

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.

In addition, this PR also:

Fixes Next should not require that eslint-config-next be installed separately #26348
Updates documentation to make it more clear what approach to take for new and existing ESLint configurations

housseindjirdeh · 2021-06-25T19:12:32Z

packages/next/lib/helpers/install.ts

@@ -0,0 +1,114 @@
+/* eslint-disable import/no-extraneous-dependencies */


This file, along with the other helper/ files in the PR, were ported over directly from 'package/create-next-app`.

If there is a preferred way to re-use the same logic without duplicating these files, let me know 🙏

housseindjirdeh · 2021-06-25T19:13:53Z

packages/next/package.json

@@ -98,6 +99,7 @@
    "pnp-webpack-plugin": "1.6.4",
    "postcss": "8.2.13",
    "process": "0.11.10",
+    "prompts": "2.4.1",


Introduced this to the repo (14.3KB gzipped) to show the prompt to the user when they run next lint.

If I shouldn't be including a dep for this, or if there's another smaller one I can use, let me know 🙏

Its actually 200KB since this is under dependencies so we could us ncc to bundle it down (maybe 100KB).

I'll defer to @timneutkens and @ijjk here

Oof, you're right. Tried nccing it but it was showing some buggy behavior when compiled unfortunately. Switched to using another library, cli-select, that's a lot lighter (28 KB). It doesn't have nearly as many downloads, but is light as it needs to be since I'm only using a single select prompt here.

Also used ncc + a dynamic import. That brought down the total increase to node_modules from 350 to 184 KB, but happy to consider any other alternatives if need be.

styfle · 2021-07-01T15:28:42Z

packages/next/lib/has-necessary-dependencies.ts

-  if (!checkTSDeps && !checkESLintDeps) {
-    return { resolved: undefined! }
+  if (!checkDeps) {
+    return { resolved: undefined!, missing: [] }


This should probably be resolved: new Map() so the consumers don't try to access a property on undefined

or not pass this prop to this function at all and handle the edge case in verifyTypeScriptSetup.ts with if statement ...

Good call @LetItRock. Removed the prop altogether along with the return statement and just handled the logic in verifyTypeScriptSetup.ts.

styfle · 2021-07-01T15:33:51Z

packages/next/lib/install-dependencies.ts

+
+  if (deps.length) {
+    console.log()
+    console.log(`Installing package${deps.length > 1 ? 's' : ''}:`)


Should this say "Installing devDependencies" instead?

Updated to say either devDependencies or dependencies :)

packages/next/lib/constants.ts

packages/next/lib/eslint/writeDefaultConfig.ts

docs/basic-features/eslint.md

test/integration/eslint/test/index.test.js

docs/basic-features/eslint.md

packages/next/cli/next-lint.ts

ijjk · 2021-07-30T16:03:26Z

Failing test suites

Commit: 25d271b

test/integration/eslint/test/index.test.js

ESLint > Next Lint > success message when no warnings or errors

Expand output

● ESLint › Next Lint › success message when no warnings or errors

expect(received).toContain(expected) // indexOf

Expected substring: "No ESLint warnings or errors"
Received string:    "info  - Using webpack 5. Reason: Enabled by default https://nextjs.org/docs/messages/webpack5
? How would you like to configure ESLint? https://nextjs.org/docs/basic-features/eslint
❯  Strict (recommended)
   Base
   Cancelwarn  - If you set up ESLint yourself, we recommend adding the Next.js ESLint plugin. See https://nextjs.org/docs/basic-features/eslint#migrating-existing-config
"

  304 |
  305 |       const output = stdout + stderr
> 306 |       expect(output).toContain('No ESLint warnings or errors')
      |                      ^
  307 |     })
  308 |
  309 |     test("don't create .eslintrc file if package.json has eslintConfig field", async () => {

  at Object.<anonymous> (integration/eslint/test/index.test.js:306:22)

packages/next/package.json

ijjk

Looks good after we update the mentioned dependencies

ijjk · 2021-08-04T21:25:36Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	housseindjirdeh/next.js eslint-update-7	Change
buildDuration	16.6s	15.8s	-827ms
buildDurationCached	3.7s	3.7s	-19ms
nodeModulesSize	50.1 MB	50.2 MB	⚠️ +47.6 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	housseindjirdeh/next.js eslint-update-7	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.938	2.872	-0.07
/ avg req/sec	850.88	870.58	+19.7
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.828	1.786	-0.04
/error-in-render avg req/sec	1367.54	1399.76	+32.22

Client Bundles (main, webpack, commons)

	vercel/next.js canary	housseindjirdeh/next.js eslint-update-7	Change
745.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	23.1 kB	23.1 kB	✓
webpack-HASH.js gzip	1.5 kB	1.5 kB	✓
Overall change	67 kB	67 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	housseindjirdeh/next.js eslint-update-7	Change
polyfills-HASH.js gzip	31.1 kB	31.1 kB	✓
Overall change	31.1 kB	31.1 kB	✓

Client Pages

	vercel/next.js canary	housseindjirdeh/next.js eslint-update-7	Change
_app-HASH.js gzip	980 B	980 B	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	312 B	312 B	✓
css-HASH.js gzip	329 B	329 B	✓
dynamic-HASH.js gzip	2.52 kB	2.52 kB	✓
head-HASH.js gzip	350 B	350 B	✓
hooks-HASH.js gzip	904 B	904 B	✓
image-HASH.js gzip	4.12 kB	4.12 kB	✓
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	1.66 kB	1.66 kB	✓
routerDirect..HASH.js gzip	319 B	319 B	✓
script-HASH.js gzip	387 B	387 B	✓
withRouter-HASH.js gzip	320 B	320 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	12.8 kB	12.8 kB	✓

Client Build Manifests

	vercel/next.js canary	housseindjirdeh/next.js eslint-update-7	Change
_buildManifest.js gzip	490 B	490 B	✓
Overall change	490 B	490 B	✓

Rendered Page Sizes

	vercel/next.js canary	housseindjirdeh/next.js eslint-update-7	Change
index.html gzip	531 B	531 B	✓
link.html gzip	542 B	542 B	✓
withRouter.html gzip	523 B	523 B	✓
Overall change	1.6 kB	1.6 kB	✓

Webpack 4 Mode (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	housseindjirdeh/next.js eslint-update-7	Change
buildDuration	13.1s	12.8s	-328ms
buildDurationCached	5.1s	4.9s	-232ms
nodeModulesSize	50.1 MB	50.2 MB	⚠️ +47.6 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	housseindjirdeh/next.js eslint-update-7	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.968	3.005	⚠️ +0.04
/ avg req/sec	842.28	832	⚠️ -10.28
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.826	1.792	-0.03
/error-in-render avg req/sec	1369.3	1394.74	+25.44

Client Bundles (main, webpack, commons)

	vercel/next.js canary	housseindjirdeh/next.js eslint-update-7	Change
17.HASH.js gzip	185 B	185 B	✓
677f882d2ed8..HASH.js gzip	14 kB	14 kB	✓
framework.HASH.js gzip	41.9 kB	41.9 kB	✓
main-HASH.js gzip	10.5 kB	10.5 kB	✓
webpack-HASH.js gzip	1.19 kB	1.19 kB	✓
Overall change	67.8 kB	67.8 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	housseindjirdeh/next.js eslint-update-7	Change
polyfills-HASH.js gzip	31.3 kB	31.3 kB	✓
Overall change	31.3 kB	31.3 kB	✓

Client Pages

	vercel/next.js canary	housseindjirdeh/next.js eslint-update-7	Change
_app-HASH.js gzip	965 B	965 B	✓
_error-HASH.js gzip	3.74 kB	3.74 kB	✓
amp-HASH.js gzip	552 B	552 B	✓
css-HASH.js gzip	333 B	333 B	✓
dynamic-HASH.js gzip	2.71 kB	2.71 kB	✓
head-HASH.js gzip	2.97 kB	2.97 kB	✓
hooks-HASH.js gzip	911 B	911 B	✓
index-HASH.js gzip	231 B	231 B	✓
link-HASH.js gzip	1.64 kB	1.64 kB	✓
routerDirect..HASH.js gzip	298 B	298 B	✓
script-HASH.js gzip	2.94 kB	2.94 kB	✓
withRouter-HASH.js gzip	294 B	294 B	✓
e025d2764813..52f.css gzip	125 B	125 B	✓
Overall change	17.7 kB	17.7 kB	✓

Client Build Manifests

	vercel/next.js canary	housseindjirdeh/next.js eslint-update-7	Change
_buildManifest.js gzip	499 B	499 B	✓
Overall change	499 B	499 B	✓

Rendered Page Sizes

	vercel/next.js canary	housseindjirdeh/next.js eslint-update-7	Change
index.html gzip	577 B	577 B	✓
link.html gzip	589 B	589 B	✓
withRouter.html gzip	570 B	570 B	✓
Overall change	1.74 kB	1.74 kB	✓

Commit: c6715b2

vercel/next.js#26584

…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

ijjk added type: documentation type: next labels Jun 24, 2021

housseindjirdeh changed the title ~~[ESLint] Introduce a new setup process when "`next lint is run for the first time~~ [ESLint] Introduce a new setup process when next lint is run for the first time Jun 24, 2021

This comment has been minimized.

Sign in to view

housseindjirdeh added the type: chrome label Jun 25, 2021

housseindjirdeh marked this pull request as ready for review June 25, 2021 19:06

housseindjirdeh requested review from divmain, ijjk, leerob, lfades, shuding and timneutkens as code owners June 25, 2021 19:06

housseindjirdeh commented Jun 25, 2021

View reviewed changes

This was referenced Jun 25, 2021

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

Closed

Generic build error when updating from 10.2.4-canary.2 to any version above #26574

Closed

This comment has been minimized.

Sign in to view

housseindjirdeh added created-by: Chrome Aurora and removed type: chrome labels Jun 30, 2021

styfle reviewed Jul 1, 2021

View reviewed changes

housseindjirdeh requested review from huozhi and padmaia as code owners July 2, 2021 20:34

This comment has been minimized.

Sign in to view

styfle reviewed Jul 29, 2021

View reviewed changes

packages/next/lib/constants.ts Outdated Show resolved Hide resolved

styfle reviewed Jul 29, 2021

View reviewed changes

packages/next/lib/eslint/writeDefaultConfig.ts Outdated Show resolved Hide resolved

styfle reviewed Jul 29, 2021

View reviewed changes

docs/basic-features/eslint.md Outdated Show resolved Hide resolved

styfle reviewed Jul 29, 2021

View reviewed changes

test/integration/eslint/test/index.test.js Outdated Show resolved Hide resolved

styfle reviewed Jul 29, 2021

View reviewed changes

docs/basic-features/eslint.md Outdated Show resolved Hide resolved

styfle reviewed Jul 29, 2021

View reviewed changes

packages/next/cli/next-lint.ts Outdated Show resolved Hide resolved

more updates per review comments

a30a405

This comment has been minimized.

Sign in to view

Merge branch 'canary' into eslint-update-7

4fa1cc5

This comment has been minimized.

Sign in to view

styfle previously approved these changes Aug 3, 2021

View reviewed changes

styfle requested review from ijjk and removed request for divmain August 3, 2021 15:18

Merge branch 'canary' into eslint-update-7

50ef69c

ijjk reviewed Aug 3, 2021

View reviewed changes

packages/next/package.json Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

ijjk reviewed Aug 3, 2021

View reviewed changes

packages/next/package.json Outdated Show resolved Hide resolved

ijjk requested changes Aug 3, 2021

View reviewed changes

more updates per review comments

f54b73f

housseindjirdeh dismissed styfle’s stale review via f54b73f August 4, 2021 19:47

Merge branch 'canary' into eslint-update-7

8429e50

This comment has been minimized.

Sign in to view

ijjk approved these changes Aug 4, 2021

View reviewed changes

Merge branch 'canary' into eslint-update-7

c6715b2

kodiakhq bot merged commit 7a1c9eb into vercel:canary Aug 4, 2021

akd-io added a commit to akd-io/create-next-stack that referenced this pull request Aug 12, 2021

Add setUpEslintStep after Create Next App changed eslint setup

5239aa4

vercel/next.js#26584

vercel locked as resolved and limited conversation to collaborators Jan 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ESLint] Introduce a new setup process when `next lint` is run for the first time #26584

[ESLint] Introduce a new setup process when `next lint` is run for the first time #26584

housseindjirdeh commented Jun 24, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

housseindjirdeh Jun 25, 2021

housseindjirdeh Jun 25, 2021

styfle Jul 1, 2021

housseindjirdeh Jul 2, 2021 •

edited

Loading

This comment has been minimized.

styfle Jul 1, 2021

LetItRock Jul 1, 2021

housseindjirdeh Jul 2, 2021

styfle Jul 1, 2021

housseindjirdeh Jul 2, 2021

This comment has been minimized.

This comment has been minimized.

ijjk commented Jul 30, 2021

This comment has been minimized.

This comment has been minimized.

ijjk left a comment

This comment has been minimized.

ijjk commented Aug 4, 2021

		@@ -0,0 +1,114 @@
		/* eslint-disable import/no-extraneous-dependencies */

[ESLint] Introduce a new setup process when next lint is run for the first time #26584

[ESLint] Introduce a new setup process when next lint is run for the first time #26584

Conversation

housseindjirdeh commented Jun 24, 2021 • edited Loading

Current behavior

New behavior

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

housseindjirdeh Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

ijjk commented Jul 30, 2021

Failing test suites

This comment has been minimized.

This comment has been minimized.

ijjk left a comment

Choose a reason for hiding this comment

This comment has been minimized.

ijjk commented Aug 4, 2021

Stats from current PR

[ESLint] Introduce a new setup process when `next lint` is run for the first time #26584

[ESLint] Introduce a new setup process when `next lint` is run for the first time #26584

housseindjirdeh commented Jun 24, 2021 •

edited

Loading

housseindjirdeh Jul 2, 2021 •

edited

Loading