Skip to content

Modernize Codebase #1397

@myfreeer

Description

@myfreeer

Dependency cleanup, dead code removal, and TypeScript target bump.
All built-in replacements are available in Node >= 18.17.0 (the current
engines minimum). Platform versions documented per item.


1. Drop mkdirp — use fs.promises.mkdir

Node availability: fs.promises.mkdir(path, { recursive: true }) — Node 10.12.0+

mkdirp is the sole export used from the package (1 import site).
mkdir with recursive: true is idempotent — it does not throw when the
directory already exists, which eliminates the need for the retry wrapper.

Current (src/io.ts)

import {mkdirp} from 'mkdirp';

export const mkdirRetry = async (dir: string, retry = 3): Promise<void> => {
  let error: unknown | void;
  for (let i = 0; i < retry; i++) {
    error = undefined;
    try {
      await mkdirp(dir);
    } catch (e) {
      error = e;
      // ...logging...
      continue;
    }
    error = undefined;
    return;
  }
  if (error) { throw error; }
};

Proposed

import {promises as fs} from 'node:fs';

export const mkdirRetry = async (dir: string): Promise<void> => {
  await fs.mkdir(dir, {recursive: true});
};

The retry loop was needed because old mkdirp could race with concurrent
calls on the same path. mkdir({recursive: true}) handles this internally.

Files changed

File Change
src/io.ts Replace import + simplify mkdirRetry
src/life-cycle/read-or-copy-local-resource.ts No change (calls mkdirRetry)
test/life-cycle/save-mock-fs.ts Mock fs.promises.mkdir instead of mkdirp
package.json Remove mkdirp from dependencies

Risk

Low. The only behavioral difference is that we no longer retry on failure,
because recursive: true does not fail on the races that mkdirRetry was
designed to handle. If filesystem errors occur (permissions, disk full), they
should surface immediately rather than be retried.


2. Drop promisify(pipeline) — use node:stream/promises

Node availability: stream/promises.pipeline — Node 15.0.0+

Current (src/life-cycle/download-streaming-resource.ts)

import {pipeline} from 'node:stream';
import {promisify} from 'node:util';
const promisifyPipeline = promisify(pipeline);

Proposed

import {pipeline} from 'node:stream/promises';

Files changed

File Change
src/life-cycle/download-streaming-resource.ts Replace import, remove promisify import, rename promisifyPipelinepipeline

Risk

None. Identical behavior.


3. Raise TypeScript target to es2022

V8/Node availability of key unlocked features:

Feature ES spec V8 version Node version
String.prototype.replaceAll ES2021 8.5 15.0.0
Promise.any ES2021 8.5 15.0.0
Logical assignment (??= ||= &&=) ES2021 8.5 15.0.0
Object.hasOwn ES2022 9.3 16.9.0
Array.prototype.at ES2022 9.2 16.6.0
Class static blocks ES2022 9.4 16.11.0
Top-level await ES2022 14.8.0
Error.cause ES2022 9.3 16.9.0

All available since Node 15–16 at the latest, well below the 18.17.0
minimum.

Change

 // tsconfig.json
-"target": "es2018",
+"target": "es2022",

What this unlocks

The target bump itself does not change existing code, but it removes the
downlevel transforms for for...of, async generators, optional catch
binding, etc. — producing smaller, faster output. Follow-up PRs can then
optionally:

  • Use replaceAll instead of .replace(/pattern/g, ...) in util.ts,
    resource.ts, save-html-to-disk.ts, options.ts (5 call sites)
  • Use Object.hasOwn instead of the hasOwnProperty utility in util.ts
    (removes the exported hasOwnProperty binding)
  • Use Error.cause in thrown errors for better debugging

These follow-up changes are optional and can be done incrementally.

Risk

Low, but only if no downstream consumer runs the compiled lib/
output on a runtime older than Node 16.11. The engines field
already enforces >=18.17.0 so npm/yarn will warn, but consumers who
ignore engine checks would break. If this is a concern, es2021 is the
conservative choice (everything above except Object.hasOwn and
Error.cause).


4. workingTasks: plain object → Map

Current (src/downloader/worker-pool.ts)

readonly workingTasks: Record<number, PendingPromise> = {};
// ...
delete this.workingTasks[message.taskId];           // line 100
this.workingTasks[task.taskId] = task as PendingPromise; // line 191
for (const taskId in this.workingTasks) { ... }     // line 211

The for...in iteration on a plain object risks prototype pollution and
lacks a hasOwnProperty guard. Numeric keys are coerced to strings.

Proposed

readonly workingTasks: Map<number, PendingPromise> = new Map();
// ...
this.workingTasks.delete(message.taskId);
this.workingTasks.set(task.taskId, task as PendingPromise);
for (const [taskId, pending] of this.workingTasks) { ... }

Files changed

File Change
src/downloader/worker-pool.ts Type + 5 usage sites

Risk

None. Map has identical semantics here and is faster for frequent
add/delete patterns.


5. Vendor css-url-parser

The package (css-url-parser@1.1.4) was last published in 2017. Its
entire implementation is one 25-line file with two regexes:

var commentRegexp = /\/\*([\s\S]*?)\*\//g;
var urlsRegexp = /(?:@import\s+)?url\s*\(\s*(("(.*?)")|('(.*?)')|(.*?))\s*\)|(?:@import\s+)(("(.*?)")|('(.*?)')|(.*?))[\s;]/ig;

Proposed

Create src/life-cycle/parse-css-urls.ts that inlines the same logic as
a typed ESM module:

const embeddedRegexp = /^data:(.*?),(.*?)/;
const commentRegexp = /\/\*([\s\S]*?)\*\//g;
const urlsRegexp = /(?:@import\s+)?url\s*\(\s*(("(.*?)")|('(.*?)')|(.*?))\s*\)|(?:@import\s+)(("(.*?)")|('(.*?)')|(.*?))[\s;]/ig;

export default function parseCssUrls(cssText: string): string[] {
  const urls: string[] = [];
  cssText = cssText.replace(commentRegexp, '');
  let match: RegExpExecArray | null;
  while ((match = urlsRegexp.exec(cssText))) {
    const url = match[3] || match[5] || match[6]
      || match[9] || match[11] || match[12];
    if (url && !embeddedRegexp.test(url.trim()) && !urls.includes(url)) {
      urls.push(url);
    }
  }
  urlsRegexp.lastIndex = 0;
  return urls;
}

Files changed

File Change
src/life-cycle/parse-css-urls.ts New file (inlined logic)
src/life-cycle/process-css.ts Import from ./parse-css-urls.js
src/life-cycle/css-url-parser.d.ts Delete (no longer needed)
package.json Remove css-url-parser from dependencies

Risk

Low. The regex logic is identical. Adding urlsRegexp.lastIndex = 0
after the loop is the only behavioral fix — the original library has a
latent bug where calling it twice without reset can skip matches due to
the g flag on the regex. Current code is not affected because a new
regex is created on each require() call in CJS, but the vendored ESM
version uses a module-level regex that persists.


6. Remove stale comments and the deprecated option

6a. Remove outdated comments

File:line Comment Reason
src/life-cycle/download-resource.ts:42 "workaround for retry premature close on node 12" Min engine is Node 18
src/life-cycle/download-resource.ts:66 "force cast for typescript 4.4" TypeScript is 5.9
src/life-cycle/download-streaming-resource.ts:58 "force cast for typescript 4.4" TypeScript is 5.9
src/life-cycle/download-streaming-resource.ts:65 "force cast for typescript" TypeScript is 5.9
src/downloader/types.ts:38 "workaround for typescript 4.1.2" TypeScript is 5.9

The comments can simply be removed. The casts themselves should stay if
still required by current TypeScript — only the outdated explanatory
comments are removed.

6b. Simplify the TS 4.1 PendingPromise workaround

In src/downloader/types.ts:37-41:

export interface PendingPromise<T = unknown, E = unknown> {
  // workaround for typescript 4.1.2
  resolve: ((value?: T | PromiseLike<T>) => void) |
    ((value: T | PromiseLike<T>) => void) ;
  reject: (reason?: E) => void;
}

The union type was needed because TS 4.1 inferred Promise constructor
callback signatures differently. With TS 5.9, this can be simplified to:

export interface PendingPromise<T = unknown, E = unknown> {
  resolve: (value: T | PromiseLike<T>) => void;
  reject: (reason?: E) => void;
}

6c. Remove deprecated waitForInitBeforeIdle

In src/options.ts:136-138:

/** @deprecated since 0.8.2 */
waitForInitBeforeIdle?: boolean;

This property is:

  • Declared in StaticDownloadOptions (1 site)
  • Never read anywhere in src/ or test/
  • Deprecated since 0.8.2, now at 0.9.0

Remove the property declaration. This is a minor breaking change for
consumers who set it (they will get a type error), but it has had no
effect since 0.8.2.

Files changed

File Change
src/life-cycle/download-resource.ts Remove 3 comments
src/life-cycle/download-streaming-resource.ts Remove 2 comments
src/downloader/types.ts Remove comment, simplify PendingPromise
src/options.ts Remove waitForInitBeforeIdle property

Risk

None for comment removal. Low for the PendingPromise simplification
(verify tests pass). Low for waitForInitBeforeIdle removal (semver
minor break, property was already non-functional).


7. Cosmetic: fix double-space typo

In src/life-cycle/read-or-copy-local-resource.ts, lines 46 and 72:

if (res.type ===  ResourceType.StreamingBinary) {

Two spaces before ResourceType. Trivial fix, no behavior change.


Implementation order

All items are independent and can ship in any order or together in a
single PR. Suggested grouping:

PR 1 — Dependency removal (items 1, 2, 5)
Removes mkdirp, css-url-parser, and the promisify wrapper. Net
removal of 2 runtime dependencies.

PR 2 — TypeScript modernization (items 3, 6)
Raises target to es2022, removes stale comments and deprecated options,
simplifies PendingPromise. Follow-up commits can adopt replaceAll /
Object.hasOwn incrementally.

PR 3 — Code quality (items 4, 7)
Map for workingTasks, double-space fix.

Interaction with existing proposals


Summary

# Item Deps removed Risk Effort
1 Drop mkdirp 1 Low ~30 min
2 Drop promisify(pipeline) 0 (cleanup) None ~5 min
3 Raise TS target to es2022 0 Low ~10 min
4 workingTasksMap 0 None ~15 min
5 Vendor css-url-parser 1 Low ~20 min
6 Remove stale comments / deprecated option 0 None–Low ~15 min
7 Fix double-space typo 0 None ~1 min

Total: 2 runtime dependencies removed, cleaner output, smaller
node_modules.

Metadata

Metadata

Assignees

No one assigned

    Labels

    BREAKINGissues that can cause breaking changesenhancementNew feature or request

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions