Repository navigation
chore: modernize toolchain (TS 6, @oclif/core 5, Node 22, test-utils 2.0.0-beta) - #431
Merged
Merged
Conversation
…2.0.0-beta) - @heroku-cli/test-utils -> 2.0.0-beta.0 - typescript -> ^6.0.3 (stable TS 6) - @oclif/core -> ^5.1.2 - @types/node -> ^22; engines.node -> >= 22 - eslint -> ^10, eslint-config-oclif -> ^7; drop @typescript-eslint/parser - .tool-versions -> nodejs 24.21.0; CI matrix -> 22.x and 24.x - tsconfig rootDirs -> rootDir (TS6 TS5011) - compatibility.test.ts: --ignoreConfig for isolated tsc (TS6 TS5112) - eslint.config.mjs: ignore examples/ and src/deprecated/; defer (off) the rules newly surfaced by eslint-config-oclif v7, to re-enable one by one
Re-enables rules deferred during the toolchain upgrade whose autofixes are purely formatting (no behavior change): - @stylistic/member-delimiter-style - @stylistic/operator-linebreak - @stylistic/lines-between-class-members - xo/import-specifier-newline - mocha/consistent-spacing-between-blocks - import-x/first Also applies the safe unicorn/prefer-string-replace-all warn-fix in unwrap.ts.
Re-enables deferred rules whose autofixes are type-level only (no runtime behavior change): @typescript-eslint/array-type, consistent-indexed-object-style, consistent-type-definitions, consistent-type-imports, no-inferrable-types, no-unnecessary-template-expression, no-unnecessary-type-assertion, non-nullable-type-assertion-style.
The packed-consumer contract previously permitted only @heroku/http-call to differ from the authoritative baseline. The toolchain upgrade intentionally bumps @oclif/core v4 -> v5, so add it to the allowed dependency-range changes.
Re-enables deferred rules whose autofixes change expressions but preserve behavior, plus one manual follow-up (removing a now-unused regex const in command.ts after prefer-string-starts-ends-with): - @typescript-eslint/prefer-regexp-exec, prefer-string-starts-ends-with - unicorn/no-negated-array-predicate, no-unnecessary-global-this, no-useless-override, prefer-continue, prefer-split-limit, prefer-unicode-code-point-escapes, consistent-conditional-object-spread
…S5112) TypeScript 6 errors with TS5112 when a file is passed on the tsc command line while a tsconfig.json is present. The rollback declaration smoke compiles a generated file in isolation, so ignore the repo config, matching the fix already applied to the credential-manager compatibility test.
Marks promise-returning functions async across api-client, login, login-state-coordinator, mutex, file, and related tests. Behavior-preserving; all 406 tests pass. Also simplifies a now-async return Promise.reject() to a throw (cascaded unicorn/no-useless-promise-resolve-reject fix).
Enables @typescript-eslint/no-confusing-void-expression, prefer-optional-chain, non-nullable-type-assertion-style, and import-x/no-duplicates. - prefer-optional-chain: input && input.trim() -> input?.trim() (prompt.ts), and &&-chains -> ?. in api-client.ts/completions.ts. - no-confusing-void-expression: shorthand arrows returning void expressions rewritten to block bodies (login.ts, yubikey.ts, and expect(() => fn()).throw chai assertions in the credential-handler tests); the two logout() void-result assertions in api-client.test.ts drop the redundant const + undefined check. - non-nullable-type-assertion-style: fetch_url as string -> fetch_url!. - import-x/no-duplicates: merged duplicate @heroku/http-call and heroku-credential-manager/login imports into single type-qualified imports. @typescript-eslint/no-restricted-types stays deferred: its Buffer -> Uint8Array fix changes data.toString() decode semantics in login.ts (risky). Reverted a spurious fixer artifact that rewrote 'null | string' to 'string | undefined' in particleboard-client.ts (dropped null from the API response type; no enabled rule requires it). Behavior-preserving; all 406 tests pass.
Enables unicorn/logical-assignment-operators, class-reference-in-static-methods, no-computed-property-existence-check, prefer-includes-over-repeated-comparisons, prefer-object-iterable-methods, prefer-promise-with-resolvers, prefer-simple-condition-first, plus @typescript-eslint/no-unnecessary-type-conversion and no-useless-assignment. Notable per-site decisions (behavior-preserving): - logical-assignment: 'if (!x) x = y' and 'x = x || y' -> 'x ||= y' (||=, not ??=, to mirror the original falsy-check exactly) across api-client, file, particleboard-client, and netrc-stub test helper. - no-computed-property-existence-check: 'key in obj' / truthy '[key]' checks -> Object.hasOwn() where entries are always truthy objects; two deliberate coverage checks in netrc-parser.test kept via documented inline disables. - prefer-promise-with-resolvers: hand-rolled deferred() helpers -> Promise.withResolvers() (Node >= 22 per engines). - class-reference-in-static-methods: RequestId.* -> this.* inside static methods. - login.ts isRaw: Boolean(process.stdin.isRaw) kept via documented inline disable (isRaw is typed boolean but is undefined for non-TTY streams; Boolean() normalizes it so setRawMode restores false — removing it broke 4 raw-mode tests). Deferred unicorn/consistent-class-member-order (high-churn member reordering, no behavior benefit). perfectionist/sort-array-includes autofix sorted the new includes() array. All 406 tests pass.
Enables n/prefer-global/url, no-dynamic-delete, no-extraneous-class, no-floating-promises, no-loop-func, switch-exhaustiveness-check, import-x/no-anonymous-default-export, unicorn/max-nested-calls, and unicorn/no-break-in-nested-loop. Behavior-preserving fixes: - n/prefer-global/url: dropped node:url imports, use the global URL (api-client, particleboard-client). - no-dynamic-delete: delete obj[key] -> Reflect.deleteProperty(obj, key) (command.ts flag parsing, and env/machines cleanup in tests). - no-floating-promises: void this.dequeue() for intentional fire-and-forget in Mutex. - no-break-in-nested-loop: inverted the condition in git.ts to drop the nested continue. - max-nested-calls: extracted declarationPath in compatibility.test. - anonymous-default-export: named the default Netrc instance before exporting. Documented inline disables (fix would change behavior/API): - no-extraneous-class on RequestId (exported static-class public API). - switch-exhaustiveness-check on process.platform (default covers unsupported platforms). - no-loop-func on loop-generated mocha tests that must read beforeEach-assigned tmpDir. Deferred n/prefer-global/buffer: the config wants Buffer imported from node:buffer (not the global), which would add imports across ~9 files for a ubiquitous global (high-churn, low value). All 406 tests pass.
Enables only-throw-error, use-unknown-in-catch-callback-variable, preserve-caught-error, and @eslint-community/eslint-comments/require-description. All fixes are behavior-preserving: narrow thrown reasons with as Error casts, attach the caught error as cause on re-thrown TypeErrors, type catch-callback params as unknown (narrowing at use sites), and add -- descriptions to existing block disable directives.
Fixes the 6 test-only violations: give void-context arrows block bodies to discard their return value, and wrap intentional fire-and-forget async work in void-ed IIFEs so the floated promise is explicit. Defers unicorn/import-style (keeps it off): converting node:path named imports to the default-import style it wants would rewrite ~112 call sites across 11 files for a purely cosmetic change.
Makes implicit toString()/join coercions explicit in template literals, which is output-identical: Date interpolations gain .toString(), and the deprecated- flag warning joins the string[] with a comma (Array.toString() already uses ",") in both the production message and its test assertions.
…ement no-array-sort: swap .sort() for .toSorted() at six fresh/throwaway-array call sites (Node 22+); output order is identical and nothing relied on in-place mutation. no-unsafe-string-replacement: pass the replacement as a () => value function so String#replace() cannot interpret $-patterns in the value. Behavior-identical (the current value has no $), and safer.
Groups the 20 rules we intentionally left off (behavior-changing fixers, no-unsafe-* typing debt, structural/high-churn, identifier-rename clusters, and intentional patterns) with the rationale for each, now that the safe re-enablement pass is complete.
Converts node:path named imports to the default-import style across src and test (import path from "node:path"; path.join/resolve/dirname at call sites). Behavior-preserving; Promise-executor resolve() params were left untouched. Renames two shadowing path parameters in package-contract.test.ts (path -> filePath/dirPath) that otherwise collided with the module binding.
jdodson
approved these changes
Oct 8, 2026
Contributor
There was a problem hiding this comment.
Looks good Eric.
AI found something that may be a blocker? Sounds bad, but may be overstated.
src/mutex.ts has confirmed regression: synchronous task throws can leave callers pending and stall queue. Fix it and add regression test before merge. src/api-client.ts:246 also needs review: attached HTTP error cause retains request headers, potentially including bearer token.
…ror cause
mutex: adding async to execute/dequeue (promise-function-async) regressed the
synchronous-throw path. A task throwing synchronously became a rejected promise
that the `void this.dequeue()` callers discard, leaving the caller pending
forever and the queue stalled (busy never resets). Rework execute to
await the task in try/catch/finally so sync throws and rejections both reach
reject() and the queue always keeps draining. Adds a regression test that
times out against the old code.
api-client: drop the {cause: error} added for preserve-caught-error on the
invalid-HEROKU_API_KEY error. `error` is an HTTPError whose request retains the
Authorization bearer token; attaching it as cause risks leaking the token to
logs/telemetry (Sentry). Restore the original cause-free throw with a documented
eslint-disable.
Contributor
Author
Review feedback addressed (commit 6188612)Both AI-flagged concerns were valid — not overstated. Fixed both: 1.
|
The strict/this-preferring batch rewrote RequestId.x -> this.x inside the static methods. RequestId is exported public API whose methods are called as RequestId.create()/track()/etc.; using this changes observable behavior: a detached method would throw (this === undefined), and a subclass would get its own per-subclass ids store instead of the shared RequestId.ids. Restore the explicit class references (matching main) and scope an eslint-disable for unicorn/class-reference-in-static-methods with that rationale.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Modernizes the build/test toolchain in preparation for the v14 release.
@heroku-cli/test-utils→2.0.0-beta.0typescript→^6.0.3(stable TS 6)@oclif/core→^5.1.2@types/node→^22;engines.node→>= 22eslint→^10,eslint-config-oclif→^7; removed@typescript-eslint/parser.tool-versions→nodejs 24.21.0; CI matrix now tests22.xand24.xtsconfig.json:rootDirs→rootDir(TS6 TS5011)compatibility.test.ts: pass--ignoreConfigto the isolatedtscinvocation (TS6 TS5112)eslint.config.mjs: ignoreexamples/**andsrc/deprecated/**; the rules newly surfaced by the eslint-config-oclif v6→v7 bump are temporarily turned off (notwarn, so--fixcan't rewrite them) and will be re-enabled one at a time in follow-up workType of Change
Patch Updates (patch semver update)
Testing
Notes: Bumps the minimum Node to 22 and TypeScript to 6.
Steps:
npm run build,npm test— 406 passing,npm run lint— 0 errors — all green locally).Related Issues
GitHub issue: #[N/A]
GUS work item: W-24447931