Skip to content

test(compat): cover Worker import conditions - #2890

Open
james-elicx wants to merge 3 commits into
codex/cover-react-version-conditionsfrom
codex/fix-import-conditions
Open

test(compat): cover Worker import conditions#2890
james-elicx wants to merge 3 commits into
codex/cover-react-version-conditionsfrom
codex/fix-import-conditions

Conversation

@james-elicx

@james-elicx james-elicx commented Aug 11, 2026

Copy link
Copy Markdown
Member

Why

The exact Next.js suite test/e2e/import-conditions/import-conditions.test.ts remains 2 of 12 passing. Its other ten assertions assume the Next compiler's per-route node/edge split and private edge-light condition. The vinext targeted harness intentionally uses the Node adapter, so its server paths correctly select node; emulating Next compiler labels there would be wrong. Cloudflare Workers instead activate the portable worker and workerd conditions.

This keeps the exact suite in the needs-vite-equivalent bucket and records the passing Vite-equivalent coverage against a real Wrangler Worker.

Coverage

  • Uses a local dependency with conditional package exports, matching the upstream resolver/externalization path.
  • Reverses worker and workerd key order across two exports so both active conditions are independently verified.
  • The equivalent Worker test covers middleware, Pages and App API routes, Pages and App rendering, client hydration, and server actions.
  • Verifies browser bundles select browser.

Validation

  • Exact Next.js v16.2.6 targeted suite: 2 passed, 10 failed; all server/middleware mismatches resolve to node, while client browser and RSC react-server behavior are correct.
  • Equivalent Worker test: tests/e2e/cloudflare-workers/route-handler-draft-cache.spec.ts passes against Wrangler (the complete spec has 5 passing tests; one is the import-condition equivalent).
  • vp test run tests/compatibility-support.test.ts: 5 passed.
  • Targeted vp check: clean.
  • Independent review: no findings after the package-exports parity gap and result-count wording were corrected.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@pkg-pr-new

pkg-pr-new Bot commented Aug 11, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2890
npm i https://pkg.pr.new/create-vinext-app@2890
npm i https://pkg.pr.new/@vinext/types@2890
npm i https://pkg.pr.new/vinext@2890

commit: 0306645

@github-actions

Copy link
Copy Markdown
Contributor
Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original
workers-cache preview production
web preview production

@ask-bonk

ask-bonk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review: test(compat): cover Worker import conditions

Reviewed the reclassification, the runtime-condition-library fixture, the App/Pages/middleware/route-handler fixtures, and the new E2E test. The coverage design is solid and the test will actually run in CI (matches **/cloudflare-workers/**/*.spec.ts in the cloudflare-workers project). vp test run tests/compatibility-support.test.ts passes (5/5).

Strengths worth calling out:

  • Reversing worker/workerd key order across the two exports subpaths genuinely verifies exports map-order priority when both conditions are active, not just that some worker file resolves. Good test design.
  • The Pages Router hydration pattern (module-level typeof document reassign of serverConditions to read SSR'd DOM text, plus suppressHydrationWarning on client spans) correctly avoids a hydration mismatch while still asserting the client gets browser.
  • The App Router client component correctly does not use suppressHydrationWarning, because its client spans start at "pending" on both server and first client render and only flip to browser in a post-hydration effect. The asymmetry vs. the Pages version is intentional and correct.

One substantive concern: is unsupported the right status?

The PR moves import-conditions from VITE_RUNTIME_CONDITIONS (needs-vite-equivalent) to NEXT_RUNTIME_CONDITION_LABELS (unsupported), while at the same time adding the Vite/Workers-equivalent coverage.

Those two actions seem to point in opposite directions. Looking at the sibling entries in suite-support.ts, the needs-vite-equivalent bucket is used precisely for "the exact Next.js suite doesn't port, but the underlying capability can/should be covered by a Vite+Workers-native test":

  • test/e2e/app-dir/worker/worker.test.tsneeds-vite-equivalent, reason "Web Worker behavior needs Vite and Rolldown coverage; Next.js deployment-token assertions are not portable."
  • test/e2e/react-version/react-version.test.tsVITE_RUNTIME_CONDITIONS (needs-vite-equivalent).

The dashboard label for unsupported is "Unsupported by vinext" (contribution-grid.tsx:70), whereas needs-vite-equivalent is "Needs Vite-equivalent coverage". But the feature here is supported — the new E2E test proves worker/workerd/browser conditions resolve correctly across middleware, Pages/App API, SSR/RSC, hydration, and server actions. The reason string itself even says "equivalent Worker coverage lives in tests/e2e/cloudflare-workers/route-handler-draft-cache.spec.ts", which is the definition of a satisfied Vite-equivalent, not an unsupported feature.

Marking a feature that vinext demonstrably supports (and now has coverage for) as "Unsupported by vinext" understates vinext's compatibility on the public dashboard and diverges from the convention the neighboring entries establish.

Two coherent options:

  1. Keep it needs-vite-equivalent (the feature needs a Vite equivalent, which this PR now provides), matching worker.test.ts/react-version.test.ts. The feature/reason copy can still explain the edge-light non-portability.
  2. If the intent is a new distinct meaning — "the exact Next.js suite is unsupportable because it asserts Next-compiler-private edge-light labels, but the capability is covered elsewhere" — then that's arguably a third semantic that neither existing label captures cleanly, and unsupported/"Unsupported by vinext" is the most misleading of the two existing choices.

I'd lean toward option 1 for consistency. At minimum, please confirm this reclassification is deliberate and note why unsupported is preferred over needs-vite-equivalent given a working equivalent now exists.

Minor: duplicated reason string

The full reason string is duplicated verbatim between suite-support.ts (the NEXT_RUNTIME_CONDITION_LABELS constant) and the new assertion in tests/compatibility-support.test.ts. That's a brittle copy — any future wording tweak requires editing both. Consider asserting on status + feature (and maybe reason.toBeTruthy()) rather than the exact multi-sentence reason, or import the constant. Not blocking.

Nothing else stands out — fixtures, middleware header injection (no matcher, so it runs for the API routes as the test assumes), and the exports layout all look correct.

@ask-bonk

ask-bonk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2890. Summary of findings:

One substantive concern — status classification:

  • The PR reclassifies import-conditions from needs-vite-equivalentunsupported while simultaneously adding the Vite/Workers-equivalent coverage. These two actions conflict.
  • The dashboard renders unsupported as "Unsupported by vinext", but the feature demonstrably is supported (the new E2E test proves worker/workerd/browser resolution across middleware, API routes, SSR/RSC, hydration, and server actions). The reason string even points at the new equivalent test — which is the definition of needs-vite-equivalent ("Needs Vite-equivalent coverage").
  • Sibling entries (worker.test.ts, react-version.test.ts) use needs-vite-equivalent for exactly this pattern. I recommended keeping it needs-vite-equivalent for consistency, or at least confirming the reclassification is deliberate.

Minor — duplicated reason string: the full multi-sentence reason is copied verbatim into the unit test assertion, which is brittle. Suggested asserting on status/feature instead of the exact string.

Strengths verified:

  • Reversing worker/workerd key order across the two exports genuinely tests exports map-order priority.
  • Hydration patterns are correct in both routers (Pages uses suppressHydrationWarning + DOM-read; App correctly omits it since spans start at "pending" on both sides).
  • The new spec actually runs in CI (cloudflare-workers project), builds a real Wrangler Worker, and middleware runs for the API routes (no matcher). tests/compatibility-support.test.ts passes 5/5.

No correctness bugs found in the fixtures or test logic.

github run

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared 0306645 against base 08819bc using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 135.2 KB 135.2 KB ⚫ -0.0%
Client entry size (gzip) vinext 122.7 KB 122.7 KB ⚫ -0.0%
Dev server cold start vinext 2.37 s 2.35 s ⚫ -0.7%
Production build time vinext 2.51 s 2.51 s ⚫ +0.0%
RSC entry closure size (gzip) vinext 114.2 KB 114.2 KB ⚫ -0.0%
Server bundle size (gzip) vinext 193.0 KB 193.0 KB ⚫ -0.0%

View detailed results and traces

🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head

@james-elicx
james-elicx force-pushed the codex/fix-import-conditions branch from f7c64b4 to 9049350 Compare August 11, 2026 04:21
@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review: test(compat): cover Worker import conditions

Re-reviewed the current diff end-to-end: the suite-support.ts reclassification, the runtime-condition-library fixture (both exports subpaths + all five condition files), the App/Pages/middleware/route-handler fixtures, the new E2E test, and the lockfile. Ran vp test run tests/compatibility-support.test.ts (5/5 pass), vp check on the changed source (clean), and pnpm install --frozen-lockfile (valid).

Prior review concerns are now resolved

The earlier bot review flagged two things against an older revision; both are addressed in the diff that will merge:

  1. Status classification. NEXT_RUNTIME_CONDITION_LABELS is status: "needs-vite-equivalent", not unsupported. This matches the worker.test.ts / react-version.test.ts convention and is the correct label given the new Worker-equivalent coverage this PR adds. No further action needed.
  2. Duplicated reason string. The unit test asserts status + feature via toMatchObject and only toContains the spec path substring, rather than pinning the full multi-sentence reason verbatim. This is appropriately non-brittle.

Verified correct

  • Exports map-order priority genuinely tested: worker-first lists worker before workerd, workerd-first reverses them, so both active conditions are independently proven to win by position, not by file existence.
  • Hydration parity is right in both routers. App Router ClientConditions.tsx uses useEffect and starts client spans at "pending" on both server and client, so no suppressHydrationWarning is needed; serverConditions crosses the RSC boundary as a serialized prop and replays identically. Pages Router uses suppressHydrationWarning + a DOM read of the SSR'd server spans, which correctly tolerates the server→browser flip.
  • Middleware runs for the API routes. No matcher is exported, so middleware executes for /api/pages-import-conditions and /api/app-import-conditions, satisfying the test's middleware: { workerFirst, workerdFirst } assertions.
  • CI wiring. The spec lives under tests/e2e/cloudflare-workers/ and will run in the cloudflare-workers Playwright project against a real Wrangler build.
  • Lockfile. pnpm install --frozen-lockfile succeeds; the file: dep is grouped with the sibling fake-css-module-lib entries as pnpm expects.

Minor nit (non-blocking): useLayoutEffect in the Pages Router SSR fixture

pages/pages-import-conditions.tsx:21 uses React.useLayoutEffect to read the client-side condition post-hydration. Pages Router pages are server-rendered, so React emits the "useLayoutEffect does nothing on the server" warning during SSR. The effect only sets client state after hydration, so React.useEffect is behaviorally identical here and avoids the console warning. The App Router client component already uses useEffect for the same purpose, so switching would also make the two fixtures consistent. Not a correctness issue and won't fail the E2E assertions.

Nothing else stands out. The coverage design is sound and the reclassification is deliberate and consistent with neighboring entries.

@ask-bonk

ask-bonk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2890.

Summary

I re-reviewed the entire current diff (18 files) and validated it against the repo (vp test, vp check, and pnpm install --frozen-lockfile all pass).

Prior bot-review concerns are already resolved in this diff:

  • The status is needs-vite-equivalent (not unsupported) — consistent with worker.test.ts/react-version.test.ts and correct given the added Worker coverage.
  • The unit test uses toMatchObject + a substring toContain, not a brittle verbatim copy of the multi-sentence reason.

Verified correct:

  • Exports map-order priority is genuinely tested by reversing worker/workerd key order across the two subpaths.
  • Hydration is right in both routers (App uses useEffect+pending; Pages uses suppressHydrationWarning+DOM read).
  • Middleware runs for the API routes (no matcher), satisfying the middleware assertions.
  • The spec runs in the cloudflare-workers Playwright project; lockfile is valid.

One minor, non-blocking nit:

  • pages/pages-import-conditions.tsx:21 uses useLayoutEffect in an SSR'd Pages Router page, which triggers React's "useLayoutEffect does nothing on the server" warning. useEffect is behaviorally identical here and matches the App Router fixture. No correctness impact.

No correctness bugs found.

github run

@james-elicx
james-elicx force-pushed the codex/fix-import-conditions branch from 9049350 to e32186d Compare August 11, 2026 04:25
@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review: test(compat): cover Worker import conditions

Reviewed the full diff (18 files): the suite-support.ts reclassification, the runtime-condition-library fixture (both exports subpaths + all five condition files), the App/Pages/middleware/route-handler fixtures, the new E2E test, and the lockfile. Verified locally:

  • vitest run tests/compatibility-support.test.ts → 5/5 pass.
  • pnpm install --frozen-lockfile → succeeds; the file: dep links into tests/fixtures/cf-app-basic/node_modules/runtime-condition-library correctly.

Prior-review concerns are resolved in this diff

  • Status classification is needs-vite-equivalent (not unsupported), matching the worker.test.ts / react-version.test.ts convention. Correct given this PR adds the Worker-equivalent coverage.
  • Reason-string brittleness is gone: the unit test uses toMatchObject on status/feature and only toContains the spec-path substring, rather than pinning the full multi-sentence reason.

Verified correct

  • Exports map-order priority is genuinely exercised: worker-first lists worker before workerd; workerd-first reverses them. With both conditions active in Workers, the first-listed key wins by position, so each condition is independently proven (not merely "some worker file resolved").
  • App Router hydration is right: ClientConditions.tsx starts client spans at "pending" on both server and client and only flips in a useEffect, so no suppressHydrationWarning is needed. serverConditions crosses the RSC boundary as a serialized prop, so the server-* spans render identically on server and client.
  • Pages Router hydration is right: suppressHydrationWarning on the client-* spans tolerates the pending → browser post-hydration flip, and the module-level DOM read of the SSR'd server-* spans keeps serverConditions consistent across hydration.
  • Server action returns the RSC-env (worker/workerd) module constants; useActionState starts at "pending" and the test clicks before asserting. Sound.
  • Middleware runs for the API routes. No matcher is exported, so /api/pages-import-conditions and /api/app-import-conditions both observe the injected x-middleware-* headers the test asserts on.
  • CI wiring. The spec lives under tests/e2e/cloudflare-workers/ and runs in the cloudflare-workers Playwright project against a real Wrangler build.

Note: the earlier bot's useLayoutEffect nit is stale

A prior comment flagged pages/pages-import-conditions.tsx:21 using React.useLayoutEffect and recommended useEffect. The committed file already uses React.useEffect (line 21) — there is nothing to change here.

Minor (non-blocking): package has no ./main export

runtime-condition-library/package.json defines only the two exports subpaths, with no "." entry or main. That's fine because nothing imports the bare specifier, but a bare import "runtime-condition-library" would fail to resolve. Adding a . export or main would make the fixture package more robust for future use. Not blocking.

No correctness bugs found. The coverage design is sound and the reclassification is deliberate and consistent with neighboring entries.

@ask-bonk

ask-bonk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2890.

Summary

I reviewed the full 18-file diff and validated it locally (vitest 5/5, pnpm install --frozen-lockfile succeeds and links the fixture package correctly).

No correctness bugs found. This is a well-constructed test-only PR.

Key findings:

  • Prior-review concerns already resolved: status is needs-vite-equivalent (not unsupported), consistent with sibling entries; and the unit test no longer pins the full reason string verbatim (uses toMatchObject + substring toContain).
  • Verified correct: exports map-order priority (reversed worker/workerd key order genuinely proves each condition wins by position); App/Pages hydration patterns; server action; middleware running for API routes (no matcher); and CI wiring under the cloudflare-workers Playwright project.
  • Flagged a stale prior comment: the earlier bot's useLayoutEffect nit at pages-import-conditions.tsx:21 is outdated — the committed file already uses useEffect.
  • One minor non-blocking note: the runtime-condition-library package defines only subpath exports with no ./main, so a bare import would fail — harmless since nothing imports the bare specifier.

github run

@james-elicx
james-elicx marked this pull request as ready for review August 11, 2026 04:31
@james-elicx

Copy link
Copy Markdown
Member Author

Independent policy/parity review at exact head e32186d6bd2dc7fa34e511a5b441c3aacdbbcc86: NO FINDINGS.

Verified independently from a clean detached exact-head checkout:

  • Required targeted wrapper reproduced the precise Next-suite baseline for test/e2e/import-conditions/import-conditions.test.ts: 2 passed, 10 failed, 0 skipped, retry 0/0.
  • Every failure is the expected Next-compiler-private condition mismatch: vinext’s targeted Node adapter selects node for server/middleware paths where Next asserts browser/edge-light; client browser and RSC react-server behavior remain correct.
  • The real Wrangler Worker equivalent passed 5/5. Its condition test covers middleware, Pages/App APIs, Pages SSR/hydration, App RSC/hydration, server actions, and browser bundles; reversed export-map order independently proves both active worker and workerd conditions.
  • Manifest status correctly remains needs-vite-equivalent, with a precise feature/reason and a focused non-brittle policy test; tests/compatibility-support.test.ts passed 5/5.
  • vp check and git diff --check pass; remote PR head and merge state remain exact/CLEAN.

…ack-import-conditions-2893

# Conflicts:
#	apps/web/app/compatibility/suite-support.ts
@james-elicx
james-elicx changed the base branch from main to codex/cover-react-version-conditions August 11, 2026 09:56
@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review: test(compat): cover Worker import conditions

Independent end-to-end review of the full 18-file diff against base codex/cover-react-version-conditions. Validated locally:

  • pnpm install --frozen-lockfile (via vp install) succeeds and links the fixture package: tests/fixtures/cf-app-basic/node_modules/runtime-condition-library → the pnpm file: store entry.
  • vp test run tests/compatibility-support.test.ts → 5/5 pass.
  • vp check on the changed source/spec/fixtures → format and lint/type-check clean.

No correctness bugs found. This is a well-constructed, test-only PR. Everything below is confirmation plus a couple of non-blocking notes.

Verified correct

  • Exports map-order priority is genuinely tested. runtime-condition-library/worker-first lists worker before workerd; workerd-first reverses them. In the Workers runtime both conditions are active, so the first-listed key wins by position → worker-first resolves worker, workerd-first resolves workerd. This proves each condition independently, not just "some worker file resolved."
  • Client selects browser. With worker/workerd inactive in the client build, both subpaths fall through to browser. The spec asserts client-* = browser for both routers.
  • App Router hydration is sound. ClientConditions.tsx starts client-* spans at "pending" on both server and client and only flips in useEffect, so no suppressHydrationWarning is needed. serverConditions crosses the RSC boundary as a serialized prop, so server-* spans render identically.
  • Pages Router hydration is sound. suppressHydrationWarning on the client-* spans tolerates the pending → browser post-hydration flip; the module-level typeof document read of the SSR'd server-* spans keeps serverConditions consistent across hydration. The committed file uses React.useEffect (the earlier bot's useLayoutEffect nit is stale).
  • Middleware runs for the API routes. No matcher/config is exported, so middleware executes for /api/pages-import-conditions and /api/app-import-conditions, and it only mutates the request headers via NextResponse.next({ request: { headers } }) — existing response-header assertions in this spec are unaffected.
  • Server action returns RSC-env constants (worker/workerd); useActionState starts at "pending" and the test clicks before asserting via auto-retrying toHaveText. No race.
  • CI wiring + real build. The spec lives under tests/e2e/cloudflare-workers/ (runs in the cloudflare-workers Playwright project) and the beforeAll does a real vp build + wrangler dev, so condition resolution is exercised against an actual Worker bundle, not mocked.
  • Status classification is needs-vite-equivalent (not unsupported), consistent with the sibling worker.test.ts / react-version.test.ts entries and correct given this PR adds the Worker-equivalent coverage. The unit test asserts status/feature via toMatchObject plus a substring toContain on the spec path, avoiding the brittle verbatim-reason pin flagged in an earlier revision.
  • Type resolution. Each exports subpath carries a types condition and the repo uses moduleResolution: "bundler", so the subpath type imports resolve.

Minor (non-blocking)

  • runtime-condition-library/package.json defines only the two subpath exports with no "."/main. Harmless — nothing imports the bare specifier — but a bare import "runtime-condition-library" would fail to resolve. Adding a . export would make the fixture more reusable. (Previously raised; noting it persists.)

Coverage design is deliberate and consistent with neighboring entries. LGTM.

@ask-bonk

ask-bonk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2890.

Summary

I independently reviewed the full 18-file diff and validated it locally:

  • pnpm install --frozen-lockfile succeeds and correctly links the fixture package.
  • vp test run tests/compatibility-support.test.ts → 5/5 pass.
  • vp check on all changed source/spec/fixture files → format + lint/type clean.

No correctness bugs found. This is a well-constructed, test-only PR.

Key confirmations:

  • Exports map-order priority is genuinely tested by reversing worker/workerd key order across the two subpaths, so each active condition is proven to win by position.
  • Hydration is correct in both routers (App uses useEffect + pending; Pages uses suppressHydrationWarning + DOM read), and the committed Pages file already uses useEffect — the earlier useLayoutEffect nit is stale.
  • Middleware runs for the API routes (no matcher) and only mutates request headers, so existing response-header assertions are unaffected.
  • Real build + CI wiring: the spec does a real vp build + wrangler dev in the cloudflare-workers Playwright project, exercising actual condition resolution.
  • Status classification is needs-vite-equivalent (correct, matches sibling entries), and the unit test avoids the brittle verbatim-reason pin.

One minor non-blocking note: the fixture package defines only subpath exports with no ./main, so a bare import would fail — harmless since nothing imports the bare specifier.

github run

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

UnknownError: ProviderInitError

github run

@ask-bonk

ask-bonk Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

@james-elicx Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

1 similar comment
@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: test(compat): cover Worker import conditions

Reviewed the full 18-file diff against base codex/cover-react-version-conditions: the suite-support.ts reclassification, the runtime-condition-library fixture (both exports subpaths + all condition files), the App/Pages/middleware/route-handler fixtures, the new E2E test, the unit test, and the lockfile. Validated locally:

  • vp install (frozen lockfile) → succeeds; the file: dep links correctly.
  • vp test run tests/compatibility-support.test.ts → 5/5 pass.
  • vp check on the changed source/spec/fixtures → format + lint/type clean.

No correctness bugs found. This is a well-constructed, test-only PR.

Verified correct

  • Exports map-order priority is genuinely tested. worker-first lists worker before workerd; workerd-first reverses them. In Workers both conditions are active, so the first-listed key wins by position → worker-first resolves worker, workerd-first resolves workerd. This proves each condition independently rather than just "some worker file resolved."
  • Client selects browser. With worker/workerd inactive in the client build, both subpaths fall through to browser; the spec asserts client-* = browser in both routers.
  • App Router hydration is sound. ClientConditions.tsx starts client-* spans at "pending" on both server and client and only flips in useEffect, so no suppressHydrationWarning is needed. serverConditions crosses the RSC boundary as a serialized prop, so server-* spans render identically.
  • Pages Router hydration is sound. suppressHydrationWarning on the client-* spans tolerates the pending → browser post-hydration flip; the module-level typeof document read of the SSR'd server-* spans keeps serverConditions consistent across hydration. The committed file uses React.useEffect (the earlier useLayoutEffect nit is stale).
  • Middleware runs for the API routes. No matcher/config is exported, so middleware executes for /api/pages-import-conditions and /api/app-import-conditions, injecting the x-middleware-* request headers via NextResponse.next({ request: { headers } }). This is the same well-tested request-header-injection path used by x-from-middleware.
  • Server action returns RSC-env constants (worker/workerd); useActionState starts at "pending" and the test asserts via auto-retrying toHaveText.
  • CI wiring + real build. The spec lives under tests/e2e/cloudflare-workers/, runs in the cloudflare-workers Playwright project, and beforeAll does a real vp build + wrangler dev, so conditions are exercised against an actual Worker bundle.
  • Status classification is needs-vite-equivalent (not unsupported), consistent with sibling worker.test.ts / react-version.test.ts entries and correct given this PR adds the Worker-equivalent coverage. The unit test asserts status/feature via toMatchObject plus substring toContain checks, avoiding the brittle verbatim-reason pin from an earlier revision.

Minor (non-blocking)

  • runtime-condition-library/package.json defines only the two subpath exports with no "."/main. Harmless — nothing imports the bare specifier — but a bare import "runtime-condition-library" would fail to resolve. Adding a . export would make the fixture more reusable. (Previously raised; persists.)

Coverage design is deliberate and consistent with neighboring entries. LGTM.

@ask-bonk

ask-bonk Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2890.

Summary

I reviewed the full 18-file diff and validated it locally (vp install frozen lockfile succeeds, vp test run tests/compatibility-support.test.ts 5/5 pass, vp check on changed files clean).

No correctness bugs found. This is a well-constructed, test-only PR.

Key confirmations:

  • Exports map-order priority is genuinely tested by reversing worker/workerd key order across the two subpaths, so each active condition is proven to win by position.
  • Hydration is correct in both routers (App uses useEffect + pending; Pages uses suppressHydrationWarning + DOM read). The useLayoutEffect nit from an earlier bot review is stale — the file uses useEffect.
  • Middleware runs for the API routes (no matcher) via the same well-tested request-header injection path as x-from-middleware.
  • Real build + CI wiring: the spec does a real vp build + wrangler dev in the cloudflare-workers Playwright project.
  • Status classification is needs-vite-equivalent (correct, matches sibling entries), and the unit test avoids the brittle verbatim-reason pin.

One minor non-blocking note: the fixture package defines only subpath exports with no ./main, so a bare import would fail — harmless since nothing imports the bare specifier.

github run

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