🍕 Harden browser-compat stubs: string_decoder, url, node: import-safety#254
Open
jjpaulino wants to merge 2 commits into
Open
🍕 Harden browser-compat stubs: string_decoder, url, node: import-safety#254jjpaulino wants to merge 2 commits into
jjpaulino wants to merge 2 commits into
Conversation
Prevent the class of edit-mode crash where a Node-builtin browser stub is import-unsafe (throws at module evaluation) — the family the safe-buffer #253 crash belongs to — and add a real-bundle smoke test so these regress in CI instead of in an editor's console. #1 CI boot-smoke test (browser-compat.bundle.test.js) - Runs the real kiln-edit plugin pipeline (browser-compat + node-resolve + commonjs) over the libraries that have broken before (safe-buffer, create-hash) and evaluates the emitted bundle in a Buffer-less VM. A stub-shape regression now fails CI. #2 Prefer real browser globals over bespoke shims, at zero bundle cost - Codify the stub import-safety invariant and add a shared preferGlobal() helper (single source of truth for "use globalThis.X, else minimal shim"). - buffer/node-fetch already did this; apply it to url (globalThis.URL / URLSearchParams). No heavyweight polyfills added. Fixes surfaced by the smoke test - string_decoder was an empty stub, but cipher-base (crypto-browserify -> create-hash/createHmac) does `new StringDecoder(enc)` inside .digest(enc): an empty stub throws "StringDecoder is not a constructor" the instant a hash/HMAC is stringified in the browser. Promoted to a minimal rich stub whose StringDecoder delegates to Buffer.toString(encoding). - node:-prefixed rich builtins (node:buffer, node:stream, ...) were listed in SIMPLE_STUBS, forcing them to empty stubs and re-opening the subclass-at-eval crash. resolveId already normalizes the node: prefix, so the explicit entries are removed; rich node: forms now route to the rich stub. Adds safe-buffer + create-hash as devDependencies (smoke-test fixtures only). Co-authored-by: Cursor <cursoragent@cursor.com>
|
coverage: 87.078% (-0.006%) from 87.084% — jordan/harden-browser-stubs into master |
The bundle smoke test guards claycli's own stubs against a fixed set of known libraries — it does not (and cannot) catch a sites change that pulls a new Node-builtin-dependent dependency into the kiln bundle, which is how edit mode actually broke. That guard belongs against a consuming site's real bundle (sites CI), not here. Removes browser-compat.bundle.test.js and its smoke-test-only devDependencies (safe-buffer, create-hash). Keeps the stub hardening it surfaced: string_decoder + url rich stubs, the node: routing fix, the import-safety invariant, and preferGlobal() — all covered by browser-compat.test.js. Co-authored-by: Cursor <cursoragent@cursor.com>
jjpaulino
changed the title
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.
What does this PR do?
Hardens the Vite browser-compat stubs so more Node-builtin usage is import-safe (evaluates without throwing at module-eval time), and fixes a real edit-mode crash in the crypto path. Same failure family as the
safe-buffer#253 crash: an incomplete stub shape — or an empty stub where a real one is needed — throws when a library subclasses/instantiates it, taking the kiln-edit bundle down before Kiln boots.string_decodercrash fix (the shippable bug). It was an empty stub, butcipher-base(the base class behindcrypto-browserify'screate-hash/createHmac) doesnew StringDecoder(enc)inside.digest(enc). An empty stub throwsStringDecoder is not a constructorthe instant a hash/HMAC is stringified in the browser — e.g.sitessubscription-stripe-plancomputing an HMAC in its Kilnsavehook. Now a minimal rich stub whoseStringDecoderdelegates toBuffer.toString(encoding).node:-prefixed rich builtins routed to empty stubs.node:buffer,node:stream, etc. were listed inSIMPLE_STUBS, soresolveIdshort-circuited them to the empty stub before the rich branch — re-opening the exact subclass-at-eval crash the rich stubs prevent.resolveIdalready normalizes thenode:prefix, so the explicit entries are removed and richnode:forms now route correctly.preferGlobal()helper ("useglobalThis.X, else a minimal function-shaped shim").buffer/node-fetchalready did this; applied it to a newurlstub (globalThis.URL/URLSearchParams). No heavyweight polyfills added — bundle stays minimal.Unit tests in
browser-compat.test.jscover all of the above.No linked issue — continuation of the Vite kiln-edit rollout (same effort as #252/#253).
Why are we doing this? Any context or related work?
#253 fixed the
bufferstub aftercrypto-browserify(added downstream innymag/sites) pulledsafe-bufferinto the kiln bundle and crashed it at module-eval. That was one instance of a broader pattern: each edit-mode code path that reaches a different Node builtin exposes the next incomplete stub. This PR hardens the next ones the crypto chain reaches (string_decoder,node:routing) and documents the invariant so new stubs are safe by construction.Where should a reviewer start?
In review order (core fix first):
string_decoderstub — the real, shippable crash fix →STRING_DECODER_STUBpreferGlobal()urlrich stub (prefersglobalThis.URL):URL_STUBnode:routing fix:SIMPLE_STUBSManual testing steps?
npx jest lib/cmd/vite/plugins/browser-compat.test.js→ green (buffer/url/string_decoder/node: routing).npm test→ lint clean, jest green.crypto-browserify(create-hash/createHmac), calling.digest('hex')in the browser threwStringDecoder is not a constructorbefore this change.Screenshots
N/A.
Additional Context
This PR does not by itself make browser crypto (
createHmac) functional.create-hash/createHmac.digest()also needs a byte-capableBuffer(writeUInt32BE,toString(enc)); with no globalBufferon the page it throwswriteUInt32BE is not a function. The minimal buffer stub can only defer to a realglobalThis.Buffer(the invariant's rule 2), and neither claycli norsitescurrently provides one. So any browser feature doing real crypto must either expose a realBufferglobal (whichpreferGlobalthen picks up automatically) or use Web Crypto (crypto.subtle). Tracked separately.An earlier revision of this PR added a claycli-side bundle "boot-smoke" test. It was removed: it only guards claycli's own stubs against a fixed library set and cannot catch a sites change pulling in a new dependency — that guard belongs against a consuming site's real kiln bundle (sites CI), not here.