Skip to content

Build: replace yargs with node:util parseArgs (preserve --no-lint-warnings) - #15473

Merged
patmmccann merged 11 commits into
masterfrom
codex/redo-pull-request-with-lint-warnings-preserved
Sep 4, 2026
Merged

patmmccann merged 11 commits into
masterfrom
codex/redo-pull-request-with-lint-warnings-preserved

Conversation

@patmmccann

@patmmccann patmmccann commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Picking up from @derdeka draft #15150

Motivation

  • Remove the legacy yargs dev-dependency in favor of Node.js util.parseArgs now available on Node ≥20.
  • Centralize CLI argument parsing for build/test tooling so all scripts share a single argv source.
  • Preserve the documented --no-lint-warnings behavior so gulp lint --no-lint-warnings continues to map to argv.lintWarnings === false.

Description

  • Replace direct require('yargs').argv usages with a shared argv produced by parseArgs in gulpHelpers.js and consumed by gulpfile.js, gulp.precompilation.js, and webpack.conf.js.
  • Add explicit parseArgs schema in gulpHelpers.js and export the parsed argv object for reuse across build scripts.
  • Preserve legacy behavior for the documented negated flag by normalizing the literal --no-lint-warnings key into argv.lintWarnings = false in gulpHelpers.js.
  • Migrate fake-server CLI parsing in test/fake-server/bundle.js and test/fake-server/index.js to util.parseArgs.
  • Remove yargs from package.json and update package-lock.json so the repository no longer depends on yargs.

Testing

  • npx eslint test/fake-server/bundle.js test/fake-server/index.js --cache --cache-strategy content — PASS.
  • node --check against modified files (gulpHelpers.js, gulpfile.js, gulp.precompilation.js, webpack.conf.js, test/fake-server/bundle.js, test/fake-server/index.js) — PASS.
  • CLI parsing and normalization smoke checks: node -e "const {argv} = require('./gulpHelpers.js'); ..." -- --no-lint-warnings and --nolint --file foo.js --modules a,b --ES5 --polyfills — PASS for preserved --no-lint-warnings behavior and general flag parsing.
  • Repository checks for leftover yargs usage (rg search) and package-lock consistency — PASS (no direct yargs imports remain and lockfile updated).
  • gulp lint --no-lint-warnings --nolintfix was exercised to confirm the --quiet path but the run failed on a pre-existing lint error (unused ConsentHandler in libraries/consentManagement/cmUtils.ts), which is unrelated to these changes and reported as a warning in this PR.

Codex Task

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9987bebd74

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread gulpHelpers.js Outdated
@barecheck

barecheck Bot commented Aug 8, 2026

Copy link
Copy Markdown

Barecheck - Code coverage report

Total: 91.29%

Your code coverage diff: 0.00% ▴

✅ All code changes are covered

@patmmccann

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8cc07c79c8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread gulpHelpers.js Outdated
Comment on lines +55 to +57
if (option) {
argv[option] = false;
delete argv[arg];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve the last boolean flag's value

When a command supplies both forms of a flag, such as --no-coverage --coverage, parseArgs stores them under separate keys while preserving the first insertion position; this post-processing therefore writes coverage = false regardless of which form appeared last. That breaks the usual last-option-wins behavior and prevents a caller from overriding an earlier wrapper/default flag, so the normalization needs to account for original token order rather than iterating the collapsed values object.

Useful? React with 👍 / 👎.

dgirardi and others added 9 commits September 3, 2026 07:07
yargs mapped --no-foo to foo=false for both the camelCase and kebab-case
spellings of foo, and let the last occurrence of a flag win. The previous
normalization only matched the kebab-case spelling (so --no-sourceMaps
silently no-opped) and always applied the negation regardless of a later
positive flag. Replay parseArgs option tokens in order instead, which
restores both behaviors, and describe the mechanism in the comment.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Negation matching was case- and hyphen-insensitive while positive flags
matched exactly, so misspellings like --no-COVERAGE toggled real options
that yargs left as unread literal keys. Match each boolean option's two
accepted spellings (camelCase and kebab-case) exactly instead, and state
in the comment that positive flags are recognized only under their
declared names. Add a build-logic spec that spawns node per case, since
the parse happens at module load.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
yargs let the last occurrence of a flag win even when it carried an
explicit value (--no-fetch --fetch=1 gave fetch=1); the replay skipped
valued tokens entirely, so a negation could never be overridden by the
=value form. Replay valued occurrences of declared boolean flags too,
assigning the value parseArgs recorded.

parseArgs' own allowNegative option is deliberately not used: it landed
in Node 20.16 while package.json supports >=20.0 (where the unknown key
is silently ignored, turning negation off with no error), and it does
not cover the kebab-case spellings.

Export BOOLEAN_OPTIONS so the spec can sweep every declared boolean in
both negation spellings; also pin the valued-override ordering, the
mixed-spelling outcome, and flags following a task word.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The kebab-case negation spelling is this code's uniform rule — for ES5,
yargs's own camelization mapped --no-es5 to an unread es5 key — and
nolint/ES5 are declared names, not camelCase renderings.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Accepting --no-source-maps while ignoring --source-maps was an asymmetry
with silent failures: the positive kebab spelling left the declared
option undefined, so a build asked for source maps shipped without them,
and a line like --no-source-maps --source-maps kept contradictory keys
where yargs gave the later positive form the win. Fold both polarities
of both spellings into one token-replay table: each declared boolean is
recognized under its declared name and its kebab-case form, negated or
not, last occurrence winning. String options and undeclared flags are
still left exactly as parseArgs parsed them.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
For repeated valued occurrences of one flag yargs collected an array
rather than taking the last value, so unconditional last-wins is this
replay's rule, not restored yargs behavior.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

This PR introduces changes that may not work on all browsers. According to Babel, the following polyfills may be needed, and they are not automatically included:

  • Changes to test/build-logic/argv_spec.mjs may need:
    • es.iterator.constructor
    • es.iterator.map
    • es.json.stringify
    • esnext.iterator.constructor
    • esnext.iterator.map
    • esnext.json.parse

The best way to address this is to provide good test coverage, as normal PR checks run unit tests on older browsers.

@patmmccann
patmmccann merged commit cbab682 into master Sep 4, 2026
115 checks passed
@patmmccann
patmmccann deleted the codex/redo-pull-request-with-lint-warnings-preserved branch September 4, 2026 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants