refactor: enhance argument parsing and add skip file configuration validation by AlexF090 · Pull Request #28 · AlexF090/outdated-plus

AlexF090 · 2026-03-12T15:38:44Z

Description

Improved CLI robustness and validation: spawn error handling, graceful shutdown on SIGINT/SIGTERM, safe redirect policy, robust parsing of package.json and .outdated-plus-skip, extended argument syntax (--key=value, multiple --skip), and faster package count calculation without npm subprocess.

Type of Change

Bugfix (non-breaking change that fixes an issue)
Feature (non-breaking change that adds functionality)
Breaking Change (fix or feature that changes existing functionality)
Documentation (changes to documentation)
Refactoring (code changes without functionality change)
Performance Improvement
Test (adding or changing tests)

Changes in Detail

What was changed?

Spawn (spawnJson/spawnText): Added child.on('error') handler and settled flag so the promise does not hang when npm is missing or spawn fails.
Graceful Shutdown: Module-wide AbortController; on SIGINT/SIGTERM, in-flight HTTP requests are aborted.
Fetch: Set redirect: 'error' to avoid redirect-based risks.
readPackageJson: Parsing with type checks and toStringRecord helper instead of blindly trusting the JSON structure.
getPackageCount: Synchronous, reads from package.json (dependencies + devDependencies) instead of spawning npm list – faster and without subprocess.
Args: Support for --key=value syntax; multiple --skip values are accumulated instead of overwritten.
Skip file: .outdated-plus-skip is validated with type guard isSkipFileConfig; invalid structure is ignored.
utils: hasKey helper and isSkipFileConfig type guard; isValidNpmRegistryResponse without as casts.
constants: NODE_MODULES_REGEX extended to .*node_modules\/(.+)$ for nested node_modules paths.
Tests: args.test, cli.test and utils.test extended (spawn error, shutdown, isSkipFileConfig, parsing).

Why was it changed?

Spawn without error handler leads to endless hang when e.g. npm is not in PATH.
Without graceful shutdown, fetches continue after Ctrl+C.
Redirects in fetch carry theoretical risks; for a registry-only CLI, redirect: 'error' is appropriate.
package.json and skip file can be manipulated or corrupted externally – validation prevents crashes or wrong data.
getPackageCount via npm list is slow and redundant when package.json is already read.
Multiple --skip values and --key=value improve CLI usability.

How was it implemented?

Spawn: Single resolve/reject via settled flag; on error event, empty object or empty string is returned.
Shutdown: New AbortController at start of run(); SIGINT/SIGTERM registered; in fetchPackageMeta the signal is attached to the request AbortController and removed in finally.
Skip file: After JSON.parse, isSkipFileConfig(parsed) is checked; configuration is used only when true.
getPackageCount: Calls readPackageJson(process.cwd()) and counts keys of dependencies and devDependencies (no duplicate filter, since packages appear in only one of the two lists).

Testing

Unit tests added/updated
Integration tests added/updated
Manual tests performed
All tests pass (npm test)
Linting passed (npm run lint)
Formatting checked (npm run format:check)

Test Scenarios

Args: --key=value, multiple --skip, defaults, sort/order/format/concurrency.
Spawn: Error event leads to immediate resolve with fallback (empty).
CLI: ProgressBar, getPackageCount, quiet mode, shutdown signal behavior.
Utils: isSkipFileConfig for valid/invalid objects, types, optional fields.

CLI Behavior

Before

# Multiple skip packages only possible like this:
outdated-plus --skip "a,b,c"

# With missing npm: CLI hangs
outdated-plus

# Package count via npm list (slow)

After

# Multiple skips via multiple flags or comma:
outdated-plus --skip a --skip b --skip c
outdated-plus --skip=a,b,c

# With missing npm: immediate exit with empty result
outdated-plus

# Package count from package.json (synchronous, fast)
# Ctrl+C aborts in-flight registry requests

Breaking Changes

No breaking changes
Breaking changes present

Migration Guide

N/A.

Checklist

Code follows project standards (ESLint, Prettier, TypeScript)
Comments added for complex logic
Documentation updated (README, etc.)
No new dependencies added (Zero Dependencies principle)
All tests pass
Self-review performed
Commit messages follow Conventional Commits

Additional Information

Related Issues

Closes #

Screenshots/Videos

N/A.

Review Notes

Spawn: Ensure only one resolve on error and close (settled).
Shutdown: AbortController per run; remove listeners in finally to avoid leaks.
getPackageCount: Counts dependencies + devDependencies; same package in both lists would be counted twice – not the case in practice, since package.json has each name in only one category.

…lidation - Updated `parseArgs` function to support `--key=value` syntax and accumulate multiple `--skip` flags. - Introduced `isSkipFileConfig` type guard to validate skip file configuration structure. - Improved error handling in `spawnJson` and `spawnText` functions to ensure graceful resolution. - Added tests for new argument parsing features and skip file configuration validation. - Updated regex pattern for `NODE_MODULES_REGEX` to better match nested dependencies.

Copilot

Pull request overview

Refactors the CLI’s argument parsing and skip-file handling to be more flexible and safer, while improving robustness of process spawning and dependency-path parsing.

Changes:

Extend parseArgs to support --key=value syntax and accumulate multiple --skip values.
Add isSkipFileConfig type guard and use it to validate .outdated-plus-skip content before reading fields.
Improve operational robustness (spawn error handling, abort/shutdown behavior) and expand test coverage for new parsing/regex behavior.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`src/args.ts`	Adds `--key=value` parsing and accumulates multiple `--skip` inputs; validates skip-file JSON via `isSkipFileConfig`.
`src/index.ts`	Improves spawn error handling, adds shutdown abort support for fetches, and hardens `package.json` reading.
`src/lib/utils.ts`	Introduces `hasKey` helper and `isSkipFileConfig` type guard; minor refactor in registry response guard.
`src/lib/constants.ts`	Updates `NODE_MODULES_REGEX` to match the last `node_modules/` segment for nested dependencies.
`tests/args.test.ts`	Adds tests for `--key=value` syntax and multi-`--skip` accumulation behavior.
`tests/utils.test.ts`	Adds tests for `isSkipFileConfig` type guard correctness.
`tests/cli.test.ts`	Adds tests for updated `NODE_MODULES_REGEX`, `readPackageJson`, and `getInstalledVersions`.

- Updated `parseArgs` function to prevent duplicate `--skip` values and handle empty values for `--key=`. - Modified `toStringRecord` function to exclude arrays from object validation. - Replaced `mkdirSync` with `mkdtempSync` in tests for better temporary directory management.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

…or handling in spawn functions

…ackageMeta tests

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Copilot · 2026-03-12T16:49:01Z

+  obj: object,
+  key: K,
+): obj is Record<K, unknown> {
+  return key in obj;


hasKey uses the in operator, which also checks the prototype chain. Since this is used for validating data parsed from untrusted JSON (registry responses / skip file), it can incorrectly treat inherited properties as present. Prefer an own-property check (e.g., Object.hasOwn(data, 'packages') / Object.hasOwn(data, 'time')) to make the type guards robust against prototype pollution / unexpected prototypes.

Suggested change

return key in obj;

return Object.prototype.hasOwnProperty.call(obj, key);

Copilot · 2026-03-12T16:49:01Z

  const showAll = Boolean(a.get('--show-all'));
  const showWanted = Boolean(a.get('--wanted'));
  const quiet = Boolean(a.get('--quiet'));
  const checkAll = Boolean(a.get('--check-all'));
  const iso = Boolean(a.get('--iso'));


With the new --key=value support, boolean options can be provided as --quiet=false / --iso=false, but the current parsing (Boolean(a.get(...))) will treat any non-empty string as true. Consider explicitly parsing string values for boolean flags (e.g., accept true|false|1|0) or rejecting --flag=value for boolean-only options to avoid surprising behavior.

Copilot · 2026-03-12T16:49:01Z

 /**
 * Spawns a command and returns its JSON output.
 *
 * @param cmd - The command to execute (e.g., 'npm').
 * @param args - Array of command-line arguments.
- * @returns Promise that resolves to the parsed JSON output, or an empty object if parsing fails.
+ * @returns Promise that resolves to the parsed JSON output, or an empty object if parsing fails. Rejects if the process fails to spawn (e.g. ENOENT).
 */
 export function spawnJson(cmd: string, args: string[]): Promise<unknown> {
-  return new Promise((resolve) => {
+  return new Promise((resolve, reject) => {
    const child = spawn(cmd, args, { stdio: ['ignore', 'pipe', 'ignore'] });
    let out = '';
+    let settled = false;
    child.stdout.on('data', (c) => {
      out += String(c);
    });
+    child.on('error', (err) => {
+      if (!settled) {
+        settled = true;
+        reject(err);
+      }
+    });


The PR description says spawn errors resolve with an empty object/string, but spawnJson/spawnText now reject on child.on('error') (and run() treats that as a fatal error / exit code 1). Please align either the implementation or the PR description so the documented behavior matches what the CLI will do when npm is missing (ENOENT).

Copilot · 2026-03-12T16:49:02Z

+  it('should throw Operation cancelled when shutdown is aborted during fetch', async () => {
+    mockFetch.mockImplementation(
+      (_url: string, opts?: { signal?: AbortSignal }) =>
+        new Promise((_, reject) => {
+          opts?.signal?.addEventListener?.('abort', () =>
+            reject(Object.assign(new Error('Aborted'), { name: 'AbortError' })),
+          );
+        }),
+    );
+
+    const { fetchPackageMeta } = await import('../src/index.js');
+
+    const p = fetchPackageMeta('test-pkg');
+    await Promise.resolve();
+    globalThis.__testAbortShutdown?.();
+
+    await expect(p).rejects.toThrow('Operation cancelled');
+    globalThis.__testResetShutdown?.();
+  });


This test mutates module-level shutdown state via __testAbortShutdown() and only resets it at the end of the test body. If an assertion fails (or the test errors before reaching the reset), later tests may observe an already-aborted shutdown controller and fail/flap. Consider moving __testResetShutdown?.() into an afterEach (or a try/finally in the test) to guarantee cleanup.

AlexF090 requested a review from Copilot March 12, 2026 15:41

Copilot started reviewing on behalf of AlexF090 March 12, 2026 15:41 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

Comment thread src/index.ts

Comment thread tests/cli.test.ts

Comment thread tests/cli.test.ts

Comment thread src/args.ts Outdated

Comment thread src/args.ts Outdated

AlexF090 requested a review from Copilot March 12, 2026 16:01

Copilot started reviewing on behalf of AlexF090 March 12, 2026 16:01 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

Comment thread src/index.ts Outdated

Comment thread src/index.ts

Comment thread src/index.ts

refactor: improve argument deduplication in parseArgs and enhance err…

19533ba

…or handling in spawn functions

AlexF090 requested a review from Copilot March 12, 2026 16:29

Copilot started reviewing on behalf of AlexF090 March 12, 2026 16:30 View session

refactor: improve fetch error handling for aborted requests in fetchP…

b4a6fbe

…ackageMeta tests

Copilot AI reviewed Mar 12, 2026

View reviewed changes

Comment thread src/index.ts

Comment thread src/index.ts

Comment thread src/index.ts Outdated

refactor: enhance test shutdown hooks and improve fetch error handling

a4960fc

AlexF090 requested a review from Copilot March 12, 2026 16:44

Copilot started reviewing on behalf of AlexF090 March 12, 2026 16:44 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

	return key in obj;
	return Object.prototype.hasOwnProperty.call(obj, key);

Conversation

AlexF090 commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change

Changes in Detail

What was changed?

Why was it changed?

How was it implemented?

Testing

Test Scenarios

CLI Behavior

Before

After

Breaking Changes

Migration Guide

Checklist

Additional Information

Related Issues

Screenshots/Videos

Review Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AlexF090 commented Mar 12, 2026 •

edited

Loading