fix(next-lint): update option --report-unused-disable-directives to --report-unused-disable-directives-severity #64405

coltonehrman · 2024-04-12T15:55:06Z

Why?

The Option name and type has been incorrect for --report-unused-disable-directives (it's likely that #61877 exposed this.

For correctness, we have updated the name to --report-unsed-disable-directives-severity → https://eslint.org/docs/v8.x/use/command-line-interface#--report-unused-disable-directives-severity.

Fixes 14.2 breaks lint option --report-unused-disable-directives #64402

This option accepts an argument, but was not looking for one which resulted in unexpected behavior.

ijjk · 2024-04-12T15:55:28Z

Allow CI Workflow Run

approve CI run for commit: 551a9d0

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

ijjk · 2024-04-12T19:22:27Z

Tests Passed

ijjk · 2024-04-12T19:23:41Z

Stats from current PR

Default Build

General Overall increase ⚠️

	vercel/next.js canary	coltonehrman/next.js fix/next-cli-lint	Change
buildDuration	14s	14s	N/A
buildDurationCached	8.4s	6.2s	N/A
nodeModulesSize	199 MB	199 MB	⚠️ +444 B
nextStartRea..uration (ms)	402ms	399ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	coltonehrman/next.js fix/next-cli-lint	Change
2453-HASH.js gzip	31.4 kB	31.4 kB	N/A
3304.HASH.js gzip	181 B	181 B	✓
3f784ff6-HASH.js gzip	53.7 kB	53.7 kB	✓
8299-HASH.js gzip	5.1 kB	5.1 kB	N/A
framework-HASH.js gzip	45.2 kB	45.2 kB	✓
main-app-HASH.js gzip	242 B	242 B	✓
main-HASH.js gzip	32.2 kB	32.2 kB	N/A
webpack-HASH.js gzip	1.68 kB	1.68 kB	N/A
Overall change	99.3 kB	99.3 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	coltonehrman/next.js fix/next-cli-lint	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	coltonehrman/next.js fix/next-cli-lint	Change
_app-HASH.js gzip	196 B	197 B	N/A
_error-HASH.js gzip	184 B	184 B	✓
amp-HASH.js gzip	505 B	505 B	✓
css-HASH.js gzip	324 B	325 B	N/A
dynamic-HASH.js gzip	2.5 kB	2.5 kB	N/A
edge-ssr-HASH.js gzip	258 B	258 B	✓
head-HASH.js gzip	352 B	352 B	✓
hooks-HASH.js gzip	370 B	371 B	N/A
image-HASH.js gzip	4.27 kB	4.27 kB	✓
index-HASH.js gzip	259 B	259 B	✓
link-HASH.js gzip	2.67 kB	2.67 kB	N/A
routerDirect..HASH.js gzip	314 B	312 B	N/A
script-HASH.js gzip	386 B	386 B	✓
withRouter-HASH.js gzip	309 B	309 B	✓
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	6.63 kB	6.63 kB	✓

Client Build Manifests

	vercel/next.js canary	coltonehrman/next.js fix/next-cli-lint	Change
_buildManifest.js gzip	483 B	485 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	coltonehrman/next.js fix/next-cli-lint	Change
index.html gzip	529 B	528 B	N/A
link.html gzip	541 B	541 B	✓
withRouter.html gzip	524 B	523 B	N/A
Overall change	541 B	541 B	✓

Edge SSR bundle Size

	vercel/next.js canary	coltonehrman/next.js fix/next-cli-lint	Change
edge-ssr.js gzip	95.6 kB	95.6 kB	N/A
page.js gzip	3.06 kB	3.06 kB	✓
Overall change	3.06 kB	3.06 kB	✓

Middleware size

	vercel/next.js canary	coltonehrman/next.js fix/next-cli-lint	Change
middleware-b..fest.js gzip	622 B	625 B	N/A
middleware-r..fest.js gzip	155 B	156 B	N/A
middleware.js gzip	25.5 kB	25.5 kB	N/A
edge-runtime..pack.js gzip	839 B	839 B	✓
Overall change	839 B	839 B	✓

Next Runtimes

	vercel/next.js canary	coltonehrman/next.js fix/next-cli-lint	Change
app-page-exp...dev.js gzip	171 kB	171 kB	✓
app-page-exp..prod.js gzip	97.4 kB	97.4 kB	✓
app-page-tur..prod.js gzip	99.2 kB	99.2 kB	✓
app-page-tur..prod.js gzip	93.4 kB	93.4 kB	✓
app-page.run...dev.js gzip	144 kB	144 kB	✓
app-page.run..prod.js gzip	91.9 kB	91.9 kB	✓
app-route-ex...dev.js gzip	21.4 kB	21.4 kB	✓
app-route-ex..prod.js gzip	15.2 kB	15.2 kB	✓
app-route-tu..prod.js gzip	15.2 kB	15.2 kB	✓
app-route-tu..prod.js gzip	14.9 kB	14.9 kB	✓
app-route.ru...dev.js gzip	21.1 kB	21.1 kB	✓
app-route.ru..prod.js gzip	14.9 kB	14.9 kB	✓
pages-api-tu..prod.js gzip	9.55 kB	9.55 kB	✓
pages-api.ru...dev.js gzip	9.82 kB	9.82 kB	✓
pages-api.ru..prod.js gzip	9.55 kB	9.55 kB	✓
pages-turbo...prod.js gzip	22.5 kB	22.5 kB	✓
pages.runtim...dev.js gzip	23.1 kB	23.1 kB	✓
pages.runtim..prod.js gzip	22.5 kB	22.5 kB	✓
server.runti..prod.js gzip	51.3 kB	51.3 kB	✓
Overall change	948 kB	948 kB	✓

build cache

	vercel/next.js canary	coltonehrman/next.js fix/next-cli-lint	Change
0.pack gzip	1.59 MB	1.58 MB	N/A
index.pack gzip	107 kB	106 kB	N/A
Overall change	0 B	0 B	✓

Diff details

Diff for middleware.js

Diff too large to display

Commit: ea5abad

samcx · 2024-04-12T19:37:07Z

@coltonehrman Thanks for this :pr: ! Adding a test to make sure we don't regress on this in the future.

coltonehrman · 2024-04-12T19:39:48Z

@coltonehrman Thanks for this ! Adding a test to make sure we don't regress on this in the future.

No problem! :) I'd be happy to add a test as well I just wasn't sure the best way to go about that. Are there any CLI/lint command test setups I can follow?

samcx · 2024-04-12T20:12:19Z

@coltonehrman it should be a Boolean actually :thinkies: → https://eslint.org/docs/latest/use/command-line-interface#--report-unused-disable-directives

test/integration/eslint/test/next-lint.test.js

coltonehrman · 2024-04-12T20:25:20Z

@coltonehrman it should be a Boolean actually → https://eslint.org/docs/latest/use/command-line-interface#--report-unused-disable-directives

I tried it as a boolean and got this error:

Invalid Options:
- 'reportUnusedDisableDirectives' must be any of "error", "warn", "off", and null

samcx · 2024-04-12T20:34:43Z

@coltonehrman Interesting—are their :docs: not updated? :dig-deep:

coltonehrman · 2024-04-12T20:36:14Z

@coltonehrman Interesting—are there not updated?

Very likely lol
I was trying to read through this eslint/eslint#15466. Almost seems like they were going back and forth on the design and possibly forgot to update the docs?

samcx · 2024-04-12T20:36:15Z

@coltonehrman Looks like this needs to be re-named to https://eslint.org/docs/latest/use/command-line-interface#--report-unused-disable-directives-severity

coltonehrman · 2024-04-12T20:36:52Z

@coltonehrman Looks like this needs to be re-named to https://eslint.org/docs/latest/use/command-line-interface#--report-unused-disable-directives-severity

What version of ESLint is Next.js using? Maybe the APIs are different?

samcx · 2024-04-12T20:41:50Z

@coltonehrman It doesn't seem like that was updated → https://eslint.org/docs/v8.x/use/command-line-interface#--report-unused-disable-directives-severity. Next.js is still on v8.

coltonehrman · 2024-04-12T20:47:38Z

@coltonehrman It doesn't seem like that was updated → https://eslint.org/docs/v8.x/use/command-line-interface#--report-unused-disable-directives-severity. Next.js is still on v8.

The docs don't seem to be in sync with the behavior then (or even perhaps the version of ESLint Next.js is using has a bug). The flag that is suppose to be boolean doesn't appear to support a boolean value when ran and results in an error. However, as you pointed out, it does appear that the *-severity flag should be used instead. Should we update Next.js to use the latest version of ESLint and then verify the flags again with the latest behavior?

samcx · 2024-04-12T20:53:14Z

@coltonehrman I'll push up the changes to make this --report-unused-disable-directives-severity with the correct choices ['error', null, 'off', 'warn']. We should update the eslint version in another PR (if there isn't already a PR for that).

packages/next/src/cli/next-lint.ts

packages/next/src/bin/next.ts

coltonehrman · 2024-04-12T21:08:33Z

@samcx Just out of curiosity, what is the reason for taking over control of the lint command via Next.js vs passing through options to the underlying tool (ie: ESLint)? I feel like this would cause more issues and be harder to maintain.

samcx · 2024-04-12T21:15:01Z

@coltonehrman That's a good question—I think the idea is we give you good defaults. So when you simply do next lint, you don't have to worry about adding your own CLI options.

samcx

:lgtm: :spiderman-pointing:

…-report-unused-disable-directives-severity (#64405) ## Why? The Option name and type has been incorrect for `--report-unused-disable-directives` (it's likely that #61877 exposed this. For correctness, we have updated the name to `--report-unsed-disable-directives-severity` → https://eslint.org/docs/v8.x/use/command-line-interface#--report-unused-disable-directives-severity. - Fixes #64402 --------- Co-authored-by: samcx <sam@vercel.com>

fix: next lint --report-unused-disabled-directives command

551a9d0

This option accepts an argument, but was not looking for one which resulted in unexpected behavior.

coltonehrman requested review from timneutkens, ijjk, shuding and samcx as code owners April 12, 2024 15:55

ijjk added the type: next label Apr 12, 2024

samcx changed the base branch from canary to 14-2-1 April 12, 2024 19:28

samcx requested review from huozhi, a team, ztanner, feedthejim and wyattjoh as code owners April 12, 2024 19:28

samcx requested review from jh3y and timeyoutakeit and removed request for a team April 12, 2024 19:28

samcx changed the base branch from 14-2-1 to canary April 12, 2024 19:33

test(next-lint): add test for verifying Option name and types

eb18a81

ijjk added the area: tests label Apr 12, 2024

eps1lon reviewed Apr 12, 2024

View reviewed changes

test/integration/eslint/test/next-lint.test.js Outdated Show resolved Hide resolved

test(next-lint): remove .only in test

363bfd8

samcx requested a review from eps1lon April 12, 2024 20:15

samcx changed the title ~~fix: next lint --report-unused-disabled-directives command~~ test(next-lint): add test to prevent regressions to next-lint option name and types Apr 12, 2024

samcx changed the title ~~test(next-lint): add test to prevent regressions to next-lint option name and types~~ fix(next-lint): update option --report-unused-disable-directives to --report-unused-disable-directives-severity Apr 12, 2024

fix(next-lint): update to --report-unused-disable-directives-severity

ef34b00

coltonehrman commented Apr 12, 2024

View reviewed changes

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

coltonehrman commented Apr 12, 2024

View reviewed changes

packages/next/src/bin/next.ts Show resolved Hide resolved

fix(next-lint): update option names

d3377b2

docs(next-lint): update help output

ea5abad

ijjk added the area: documentation label Apr 12, 2024

samcx approved these changes Apr 12, 2024

View reviewed changes

samcx enabled auto-merge (squash) April 12, 2024 22:06

samcx merged commit 9994e74 into vercel:canary Apr 12, 2024
73 checks passed

github-actions bot added the locked label Apr 27, 2024

github-actions bot locked as resolved and limited conversation to collaborators Apr 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(next-lint): update option --report-unused-disable-directives to --report-unused-disable-directives-severity #64405

fix(next-lint): update option --report-unused-disable-directives to --report-unused-disable-directives-severity #64405

coltonehrman commented Apr 12, 2024 •

edited by samcx

Loading

ijjk commented Apr 12, 2024 •

edited by samcx

Loading

ijjk commented Apr 12, 2024 •

edited

Loading

ijjk commented Apr 12, 2024 •

edited

Loading

samcx commented Apr 12, 2024

coltonehrman commented Apr 12, 2024

samcx commented Apr 12, 2024

coltonehrman commented Apr 12, 2024

samcx commented Apr 12, 2024 •

edited

Loading

coltonehrman commented Apr 12, 2024

samcx commented Apr 12, 2024

coltonehrman commented Apr 12, 2024

samcx commented Apr 12, 2024

coltonehrman commented Apr 12, 2024 •

edited

Loading

samcx commented Apr 12, 2024

coltonehrman commented Apr 12, 2024

samcx commented Apr 12, 2024

samcx left a comment •

edited

Loading

fix(next-lint): update option --report-unused-disable-directives to --report-unused-disable-directives-severity #64405

fix(next-lint): update option --report-unused-disable-directives to --report-unused-disable-directives-severity #64405

Conversation

coltonehrman commented Apr 12, 2024 • edited by samcx Loading

Why?

ijjk commented Apr 12, 2024 • edited by samcx Loading

ijjk commented Apr 12, 2024 • edited Loading

Tests Passed

ijjk commented Apr 12, 2024 • edited Loading

Stats from current PR

samcx commented Apr 12, 2024

coltonehrman commented Apr 12, 2024

samcx commented Apr 12, 2024

coltonehrman commented Apr 12, 2024

samcx commented Apr 12, 2024 • edited Loading

coltonehrman commented Apr 12, 2024

samcx commented Apr 12, 2024

coltonehrman commented Apr 12, 2024

samcx commented Apr 12, 2024

coltonehrman commented Apr 12, 2024 • edited Loading

samcx commented Apr 12, 2024

coltonehrman commented Apr 12, 2024

samcx commented Apr 12, 2024

samcx left a comment • edited Loading

Choose a reason for hiding this comment

coltonehrman commented Apr 12, 2024 •

edited by samcx

Loading

ijjk commented Apr 12, 2024 •

edited by samcx

Loading

ijjk commented Apr 12, 2024 •

edited

Loading

ijjk commented Apr 12, 2024 •

edited

Loading

samcx commented Apr 12, 2024 •

edited

Loading

coltonehrman commented Apr 12, 2024 •

edited

Loading

samcx left a comment •

edited

Loading