Skip to content

Fix ESM conversion correctness ahead of launch - #496

Merged
mjackson merged 11 commits into
mainfrom
mjackson/esm-conversion-review-ef82fd
Aug 26, 2026
Merged

mjackson merged 11 commits into
mainfrom
mjackson/esm-conversion-review-ef82fd

Conversation

@mjackson

Copy link
Copy Markdown
Member

Pre-launch audit of esm.unpkg.com found several verified correctness bugs in the ESM conversion pipeline plus esm.sh legacy baggage worth shedding while the service is still in beta. This PR fixes all of them. Every fix was verified end-to-end against a local build of the service, and the full repo suite passes.

Correctness fixes

  • CJS interop rework (f5cf2345): CJS entries are built through a synthetic ESM interop entry instead of regex-splicing named exports onto esbuild output. Fixes: wrong default export for Babel-compiled packages (exports.default now unwraps via __toESM), ?min silently dropping all CJS named exports, inline sourcemaps being truncated on CJS builds, and cross-package CJS reexports losing named exports (import { Buffer } from "safe-buffer" now works via export * from).
  • Shared module instances (85ac2793): bare self-references stay external and version-pinned instead of being bundled into subpath builds. preact/hooks previously inlined a private copy of preact core, breaking hooks whenever the app also imported preact.
  • Node builtin polyfills (b849d181): the emitted @jspm/core@2/nodelibs/browser/* URLs never resolved through @jspm/core's exports map, so every builtin polyfill import 404ed in production. Polyfill URLs now use the exports-map subpath and pin the resolved version; the map covers the full jspm nodelibs set (previously querystring, vm, tty, constants etc. resolved to unrelated same-named npm packages); unpolyfillable builtins and unknown node: specifiers return a clear 422 instead of reaching the browser as unresolvable specifiers.
  • Dependency specifiers (293db50e): npm: aliases resolve to the aliased package (previously emitted broken URLs like /string-width-cjs@npm:string-width@^4.2.0), workspace/git/URL specifiers fall back to the latest published version, and package names are no longer force-lowercased — the registry is case-sensitive and /JSONStream previously served the unrelated jsonstream package.

API surface cuts (beta breaking changes)

  • 822bc6c6 removes ?bundle, ?standalone, ?no-bundle, ?keep-names, ?ignore-annotations (now 400 with a clear diagnostic), the denonext target, and the &query-in-path syntax. The bundle-mode params were unimplemented no-ops and ?no-bundle produced broken output for CJS. ?meta no longer reports a build.bundle field and the X-UNPKG-Bundle-Mode header is gone.

Performance

  • 961cc1c3: dependency and polyfill URLs are emitted in canonical form (target included, params sorted), eliminating a 301 redirect on every dependency import. Adds an in-process packument cache and parallelizes specifier rewriting (previously serial multi-MB packument fetches per import).

Coverage

  • e7d0ad34: new browser-smoke and corpus cases for the exact blind spots that let these bugs through: root+subpath shared instances, ?min named imports, default-export unwrapping, cross-package reexports, npm: aliases, and uppercase package names.

After merge

  • Deploy both the esm worker and the fly origin — the beta currently runs older code.
  • Re-run pnpm test:esm-compat and the browser smoke against the fresh deployment before announcing.

Feed esbuild a synthetic ESM entry for CommonJS modules instead of
regex-splicing named exports onto the bundled output. esbuild's __toESM
interop now resolves the default export (unwrapping exports.default for
__esModule modules), named exports are real ESM exports that survive
minification, inline sourcemaps are no longer truncated, and reexports
of other packages become export * statements that resolve at runtime.
Bundling a subpath's bare self-reference produced a private copy of the
package core, so preact/hooks and preact ran as two module instances.
Keep bare self-references external, pin them to the version being built,
and skip the registry lookup for exact dependency versions.
The emitted @jspm/core@2/nodelibs/browser/* URLs never resolved through
@jspm/core's exports map (the wildcard doubled the browser segment), so
every polyfill import 404ed. Emit the bare nodelibs subpath instead and
let the exports map pick the browser variant. Cover the full set of
builtins jspm provides so bare specifiers like querystring, vm, and tty
polyfill instead of resolving to unrelated same-named npm packages, and
reject any remaining builtin (including unknown node: specifiers, which
previously passed through as unresolvable URLs) with a clear error.
Deletes the hardNodeBuiltins list, which was fully shadowed by the
polyfill map and unreachable.
Drop the ?bundle, ?standalone, ?no-bundle, ?keep-names, and
?ignore-annotations params (400 with a clear diagnostic), the denonext
target, and the &query-in-path syntax. The bundle-mode params were
no-ops (the bundler never consulted them), no-bundle produced broken
output for CommonJS sources, and each param multiplied cache keys.
Builds now always use smart bundling; ?external covers dependency
control. Corpus cases and docs updated to match.
Dependency and polyfill URLs now include the target param and use the
canonical sorted param order, so browsers no longer pay a 301 redirect
on every dependency import. Polyfill URLs pin the resolved @jspm/core
version for deterministic builds. Adds a short-lived in-process
packument cache and parallelizes specifier rewriting, which previously
fetched full packuments serially per import.
Dependency versions like npm:string-width@^4.2.0, workspace: ranges,
and git/URL specifiers previously produced broken URLs such as
/string-width-cjs@npm:string-width@^4.2.0. npm: aliases now resolve to
the aliased package and protocol specifiers fall back to the latest
published version.

Package names are also no longer force-lowercased: the registry is
case-sensitive and legacy uppercase packages (JSONStream) often coexist
with unrelated lowercase ones, so the old behavior silently served the
wrong package. Lookups try the name as given and fall back to
lowercase, and requests redirect to the registry's canonical casing.
New browser smoke cases: preact root + hooks sharing one module
instance, minified CommonJS named imports, Babel default-export
unwrapping, and cross-package reexports. New seed corpus cases for the
same classes plus npm: alias dependencies and legacy uppercase package
names — the blind spots that let the interop bugs through.
Build artifacts, CSS modules, and worker wrappers are only served at
exact-version canonical URLs, so serve them with immutable year-long
caching instead of the 60s/300s TTL that forced browsers to refetch
every module each minute. Redirects, metadata, and errors stay
short-lived so resolution changes roll out quickly.

?meta integrity is now computed from the same bytes the module URL
serves: it prefers the edge-cached artifact and stores freshly built
artifacts under the module URL, so the SRI hash handed to users matches
what a subsequent module request in the same colo returns.
Correctness:
- Reject ?meta combined with ?worker/?css/?module; the meta path was
  building the plain artifact and caching it (immutable) under the
  module URL those params actually route differently, poisoning the
  edge cache. The integrity cache also now skips development/test modes.
- Bare self-references that resolve to the entry being built are
  bundled again (externalizing them made a module import its own URL
  mid-evaluation), and blocked exports-map subpaths fail the build with
  a clear diagnostic again. Extensionless spellings of the entry serve
  the in-memory entry code.
- The require shim keeps the full namespace for __esModule modules
  instead of double-unwrapping exports.default, so required Babel-built
  dependencies keep their named properties.
- Package-name lookups no longer hit Object.prototype members: the
  polyfill table is a Map and dependency/alias/override lookups use
  own-property access (a dependency named 'constructor' previously
  crashed the build).
- resolveDependencyVersion returns semver's cleaned form (v1.2.3 ->
  1.2.3), never throws (a registry failure degrades to the raw range),
  and unpolyfilled or unknown node builtins map to @jspm/core's empty
  stub instead of failing the whole build.
- Builds whose dependency URLs could not all be pinned are served with
  the short cache policy instead of immutable, so degraded artifacts
  heal. Unknown query params are stripped during URL normalization so
  they cannot multiply immutable cache entries or force extra builds.
- The registry doc's name is only trusted as a case variant of the
  requested package, and the files service validates targets instead of
  passing unknown strings to esbuild.

Efficiency:
- buildEsmModule extracts the tarball once and reads everything from
  the extracted directory (previously 4-8 tarball downloads per build).
- The packument cache uses abbreviated registry documents, evicts
  oldest-first instead of clearing wholesale, and only ever evicts its
  own entry on failure.
- Module, raw-file, and transform responses stream instead of
  buffering.

Consistency:
- normalizeSearchParams is exported from unpkg-worker and used by the
  esm worker and dependency-URL rewriting, so canonical param ordering
  has one implementation.
- Removed the stale path-query corpus case, restored the corpora to
  prettier formatting, dropped empty scenario arrays in the generator,
  and updated the www docs for the removed params.
- eval and arguments cannot be import bindings; drop them from CommonJS
  named exports instead of generating a syntactically invalid interop
  entry that failed the whole build.
- Restore .cjs (and .json) probing for extensionless entry and require
  resolution, lost in the single-extraction refactor.
- The lowercase package-name fallback only fires on a definitive 404;
  a transient registry error no longer silently resolves an unrelated
  same-named lowercase package. Dependency version resolution drops the
  fallback entirely since published package.json files already carry
  registry-canonical names.
- Canonicalize flag param values (?min=1 -> ?min=) and comma-list
  params (conditions/exports/external order and duplicates) during URL
  normalization so equivalent spellings share one immutable cache entry
  instead of forcing separate year-long builds.
- Test hygiene: the module-level packument cache is cleared after the
  rewrite suite and the mocked edge-cache store is cleared per test, so
  mocked registry data cannot leak across test files.
@mjackson

Copy link
Copy Markdown
Member Author

Ran a max-effort local adversarial review over this PR (10 finder angles + verification + a gap sweep). 21 verified findings, all fixed in three follow-up commits:

3341472a — Cache pinned artifacts as immutable content (the remaining item from the audit): exact-version artifacts, CSS modules, and worker wrappers serve max-age=31536000, immutable; redirects/metadata/errors stay short-lived; ?meta integrity is computed from the same bytes the module URL serves (edge-cache aligned).

da75c83a — Adversarial review fixes, the highlights:

  • ?meta&worker / ?meta&css could poison the edge cache with an immutable raw artifact under the worker/css URL — those combinations are now rejected, and the integrity cache skips dev/test modes.
  • The require shim double-unwrapped __esModule dependencies (a regression from the interop rework) — it now keeps the namespace for __esModule modules.
  • Object.prototype key collisions: a dependency literally named constructor crashed builds (polyfill table is now a Map; record lookups use own-property access).
  • Self-references resolving to the entry being built are bundled again (externalizing them created a self-import cycle), and blocked exports-map subpaths fail with a clear diagnostic again.
  • Degraded builds (registry failure → unpinned dep URLs) now serve with the short cache policy instead of immutable, so they heal; unknown query params are stripped so they can't multiply immutable cache entries.
  • buildEsmModule extracts the tarball once instead of downloading it 4–8× per build; the packument cache uses abbreviated documents with oldest-first eviction; module/raw/transform responses stream.

b70eefb8 — Sweep fixes: exports.eval no longer generates a syntactically invalid interop entry; .cjs/.json extension probing restored; the lowercase package fallback only fires on a definitive 404 (a transient 5xx can no longer resolve an unrelated same-named package); flag/list param values canonicalize into one cache key; test hygiene for the module-level caches.

Full suite: 312 tests, 0 failures across all packages. Post-merge: deploy both workers + fly origin (and purge the CF cache — old cached artifacts emit dep URLs with now-removed params), then re-run pnpm test:esm-compat and the browser smoke against the fresh deployment.

- Make the default target implicit in canonical URLs. Browsers key the
  module map by request URL, so /preact@10.26.4 redirecting to a
  ?target=es2022 variant while rewritten dependency imports used the
  variant directly loaded the same module twice — preact hooks got a
  different preact instance than the page's own import and crashed.
  Default-target URLs now serve directly (also removing the redirect
  hop entirely); only non-default targets appear in URLs.
- Shim the Buffer and process globals in browser builds via esbuild
  inject (and define global as globalThis). CJS packages that reference
  the Node globals without requiring them (JSONStream) previously threw
  ReferenceError in browsers. The shim imports are marked side-effect
  free so bundles that never touch them stay unchanged, and the local
  'process' function preact defines is left alone.
- Forward the worker's own origin to /transform so inline-script
  transforms emit imports against the serving deployment instead of the
  production origin — dev and CI runs are now hermetic.
- Make the is-hotkey smoke assertion platform-independent (shift+s;
  'mod' resolves to meta on macOS but ctrl on Linux CI) and point the
  two redirect-query-preservation corpus cases at range versions, since
  their exact-version URLs no longer redirect.

Verified against the CI-equivalent local stack: seed compat suite 37/37,
browser smoke 35/35.
@mjackson

Copy link
Copy Markdown
Member Author

CI is green as of 16650b01. The browser-smoke failures it caught were real product bugs, and one forced a design correction worth calling out:

Canonical URLs no longer carry target=es2022 — the default target is implicit. The earlier commits canonicalized toward an explicit target param; the smoke suite proved that wrong in a way unit tests couldn't: browsers key the module map by request URL, so import "/preact@10.26.4" (which redirected to the ?target=es2022 variant) and a dependency artifact's direct import of that variant loaded two preact instances, breaking hooks at runtime. With the default implicit, the bare URL is itself canonical — no redirect at all for the common case, and one shared module identity. Non-default targets still appear explicitly and stay consistent across a dependency graph.

Also in this commit: browser builds shim the Buffer/process globals via esbuild inject (JSONStream-class CJS packages that reference globals without requiring them now work), global is defined as globalThis, inline /transform responses now emit imports against the serving origin instead of hardcoding production (dev/CI hermeticity), and two smoke/corpus cases were corrected (a platform-dependent mod+s assertion; redirect-preservation cases moved to range versions since exact-version URLs no longer redirect).

Local verification against the CI-equivalent stack: seed compat suite 37/37, browser smoke 35/35, unit suites 314/314.

@mjackson
mjackson merged commit 55c6e6e into main Aug 26, 2026
1 check 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.

1 participant