Skip to content

Harden build/publish, algorithm, and tests without changing the API#2

Merged
Acro merged 2 commits into
masterfrom
claude/npm-package-review-0xjzp8
Jul 12, 2026
Merged

Harden build/publish, algorithm, and tests without changing the API#2
Acro merged 2 commits into
masterfrom
claude/npm-package-review-0xjzp8

Conversation

@Acro

@Acro Acro commented Jul 12, 2026

Copy link
Copy Markdown
Owner

Backward-compatible hardening of the library. The public default-export signature, the default concurrency of 5, input-order results, Promise.all error semantics, and as const tuple return-type inference are all unchanged (verified with a type test). Version bumped to 2.2.0 (additive feature + fixes).

Critical fixes (were quietly breaking or fragile)

  • Publish never ran tsc. "prepublish": "tsc" sat outside scripts (npm ignores it) and prepublish is deprecated. Build now runs via prepare + a build script.
  • dist/ shipping was non-deterministic. dist/ is git-ignored with no .npmignore and no files field, so npm falls back to .gitignore. Added "files": ["dist"]; npm pack --dry-run now yields a deterministic 5-file / ~5.6 kB tarball (the published 2.1.0 ships 10 files including src/, test/, and tsconfig.json).
  • License mismatch. LICENSE is MIT but package.json said ISC — and npm currently advertises ISC. Set to MIT to match the actual license file.

Correctness / robustness

  • Concurrency limit validation. limit || 5 with no validation made limit = -1 / 2.5 throw a cryptic RangeError. Now normalized: non-positive / non-integer values fall back to the default of 5 (preserves the old 0 → 5 behaviour). limit is now optional.
  • Iterative worker pool. Replaced async recursion (which retained a promise chain per worker) with a while loop. Same output; no retained chains. Tested with 1000 jobs.

Quality / DX

  • Added a dependency-free 11-case assertion test suite — including an explicit sustained max concurrency test (proves a finished job is replaced immediately, i.e. a true sliding pool, not batching) and wired up npm test.
  • Added a GitHub Actions CI matrix (Node 16/18/20/22).
  • Added a named parallel export and DEFAULT_CONCURRENCY alongside the unchanged default export.
  • Modernized tsconfig (dropped deprecated/unused options), varconst/let, refreshed README to document ordering and error behaviour.

Backward-compatibility: verified against the real dependent

The only published dependent is @paperbits/core (@paperbits/amp declares the dep but has no call site). It pins ^2.1.0, so it will pick up 2.2.0 automatically. Its actual usage, at all 3 call sites, is:

import parallel from "await-parallel-limit";      // default export
await parallel(tasks, maxParallelPublisingTasks);  // maxParallelPublisingTasks = 30; return value discarded
  • Default export, positive-integer limit (30), result never read → every changed surface (optional limit, 0→5 fallback, tuple typing, named export) is a no-op for them.
  • Head-to-head runtime diff (published 2.1.0 vs. this branch, 100 thunks @ limit 30): happy path identical — peak concurrency 30, all 100 complete, input order preserved; error path — both fail-fast with the same error at the same point.

Not included (deliberately — breaking or judgment calls)

  • AbortSignal support and a settle-all / stopOnError: false mode (change/extend semantics — worth a 3.0 or opt-in option).
  • Dual ESM/CJS build with an exports map.

Testing

  • npm test → 11/11 passing (build + assertions).
  • npm pack --dry-run → tarball contains dist/index.js, dist/index.d.ts, LICENSE, README.md, package.json.
  • Type test confirms parallel(jobs as const, 2) still infers an ordered tuple.
  • Runtime-equivalence check vs. published 2.1.0 on the @paperbits/core call shape.

🤖 Generated with Claude Code

claude added 2 commits July 12, 2026 12:25
Backward-compatible improvements to the library:

- Fix broken publish flow: `prepublish` was outside `scripts` (ignored by
  npm) and is deprecated. Build now runs via `prepare`, and a `files`
  allowlist guarantees `dist/` ships even though it is git-ignored.
- Correct license metadata (package.json said ISC; LICENSE is MIT).
- Rewrite the worker pool as an iterative loop instead of async recursion,
  avoiding retained promise chains for large job arrays.
- Normalise the concurrency limit: non-positive / non-integer values fall
  back to the default of 5 (preserves the old `limit || 5` behaviour and
  no longer throws a RangeError on negatives/floats). `limit` is now
  optional with a default.
- Add a named `parallel` export and `DEFAULT_CONCURRENCY` alongside the
  existing default export (default export unchanged).
- Add a real, dependency-free assertion test suite and wire up `npm test`
  plus a GitHub Actions CI matrix (Node 16/18/20/22).
- Modernise tsconfig (drop deprecated/unused options) and refresh README.

Tuple return-type inference for `as const` job arrays is preserved.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013eq96Mbqa2dBCAN3YCyrDK
…ching)

Adds a deterministic test using deferred promises: after resolving only the
first of two in-flight jobs, the third job must start immediately while the
second is still running. This distinguishes a true sliding worker pool from a
batching implementation (which would wait for the whole chunk). Verified to
fail against a batching impl and pass against the current loop-based pool.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013eq96Mbqa2dBCAN3YCyrDK
@Acro
Acro merged commit 3c0a2af into master Jul 12, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants