test: prove edge conditional-export resolution + verify with the real Workers bundler - #122
Merged
Merged
Conversation
…eal Workers bundler Closes the last gap from the external review: our edge coverage bundled dist/protect.edge.js DIRECTLY, which proves the artifact is edge-clean but not that a consumer ever reaches it — a mis-ordered or mistyped `exports` condition would silently hand an edge bundler the Node build. - tests/protect/edge-export-resolution.test.ts imports the real specifier (`@patchstack/connect/protect`) from a fixture with the package linked into node_modules, and resolves it under workerd / worker / edge-light / deno / browser, asserting each lands on the edge artifact. The CONTROL is what makes it meaningful: with no edge condition the same import resolves to the Node build and FAILS to bundle for a Node-free target, so a pass is caused by the condition rather than a lenient target. Mutation-checked: pointing `workerd` at dist/protect.js makes it fail. (platform 'neutral' on purpose — 'browser' would inject the `browser` condition and mask whether the edge conditions themselves work.) - scripts/verify-edge-platform.mjs (`npm run verify:edge`) compiles a real Worker with the actual Cloudflare toolchain (`wrangler deploy --dry-run`) and asserts wrangler selected dist/protect.edge.js and emitted a bundle with no Node builtins. Verified locally: wrangler 4 compiles it, 134.90 KiB, zero Node imports. It downloads wrangler and shells out to a platform bundler, so it is deliberately NOT in `npm test` (which CI runs on four Node versions) — the two suite-level tests cover the same property cheaply and this is the end-to-end confirmation for a release job. A native `next build` fixture remains the one uncovered variant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Adds an end-to-end edge-bundler verification script with complementary export-resolution tests. 🎯 Quality: 100% Elite · 📦 Size: Medium 📈 This month: Your 55th PR — above team average · Averaging Excellent |
Contributor
Author
|
/review |
daniloradovic
approved these changes
Aug 13, 2026
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
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.
Closes the last gap the external review left open.
The gap
Our edge coverage bundled
dist/protect.edge.jsdirectly. That proves the artifact is edge-clean, but not that a consumer ever reaches it — a mis-ordered or mistypedexportscondition would silently hand an edge bundler the Node build, and every existing test would still pass.1. Resolution test (in the suite)
tests/protect/edge-export-resolution.test.tsimports the real specifier@patchstack/connect/protectfrom a fixture with the package linked intonode_modules, and resolves it underworkerd/worker/edge-light/deno/browser— asserting each lands on the edge artifact.The control is what makes it meaningful: with no edge condition, the same import resolves to the Node build and fails to bundle for a Node-free target. So a pass is caused by the condition, not by a lenient target. Uses
platform: 'neutral'deliberately —'browser'would inject thebrowsercondition and mask whether the edge conditions themselves work.Mutation-checked: pointing
workerdat./dist/protect.jsmakes the test fail; restoring it makes it pass. It genuinely guards the regression.2. Real platform verification (opt-in)
scripts/verify-edge-platform.mjs(npm run verify:edge) compiles a real Worker with the actual Cloudflare toolchain (wrangler deploy --dry-run) and asserts wrangler selecteddist/protect.edge.jsand emitted a bundle with no Node builtins.Verified locally — wrangler 4 compiles it:
Total Upload: 134.90 KiB, edge branch selected, zero Node imports.It downloads wrangler and shells out to a platform bundler, so it is deliberately not in
npm test(which CI runs across four Node versions). The two suite-level tests cover the same property cheaply; this is the end-to-end confirmation, suitable for a release job.Remaining
A native
next buildEdge-middleware fixture is the one uncovered variant. Wrangler exercises the same constraint (workerd conditions + no Node builtins) at a fraction of the install cost, so this is a lower marginal return — noted rather than done.779 tests green; typecheck + build clean.