test(compat): cover Worker import conditions - #2890
Conversation
|
/bigbonk review for issues |
commit: |
|
Review: test(compat): cover Worker import conditionsReviewed the reclassification, the Strengths worth calling out:
One substantive concern: is
|
|
Review posted to PR #2890. Summary of findings: One substantive concern — status classification:
Minor — duplicated reason string: the full multi-sentence Strengths verified:
No correctness bugs found in the fixtures or test logic. |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
f7c64b4 to
9049350
Compare
|
/bigbonk review for issues |
Review: test(compat): cover Worker import conditionsRe-reviewed the current diff end-to-end: the Prior review concerns are now resolvedThe earlier bot review flagged two things against an older revision; both are addressed in the diff that will merge:
Verified correct
Minor nit (non-blocking):
|
|
Review posted to PR #2890. SummaryI re-reviewed the entire current diff (18 files) and validated it against the repo ( Prior bot-review concerns are already resolved in this diff:
Verified correct:
One minor, non-blocking nit:
No correctness bugs found. |
9049350 to
e32186d
Compare
|
/bigbonk review for issues |
Review: test(compat): cover Worker import conditionsReviewed the full diff (18 files): the
Prior-review concerns are resolved in this diff
Verified correct
Note: the earlier bot's
|
|
Review posted to PR #2890. SummaryI reviewed the full 18-file diff and validated it locally ( No correctness bugs found. This is a well-constructed test-only PR. Key findings:
|
|
Independent policy/parity review at exact head Verified independently from a clean detached exact-head checkout:
|
…ack-import-conditions-2893 # Conflicts: # apps/web/app/compatibility/suite-support.ts
|
/bigbonk review for issues |
Review: test(compat): cover Worker import conditionsIndependent end-to-end review of the full 18-file diff against base
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
Minor (non-blocking)
Coverage design is deliberate and consistent with neighboring entries. LGTM. |
|
Review posted to PR #2890. SummaryI independently reviewed the full 18-file diff and validated it locally:
No correctness bugs found. This is a well-constructed, test-only PR. Key confirmations:
One minor non-blocking note: the fixture package defines only subpath |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
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; thefile:dep links correctly.vp test run tests/compatibility-support.test.ts→ 5/5 pass.vp checkon 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-firstlistsworkerbeforeworkerd;workerd-firstreverses them. In Workers both conditions are active, so the first-listed key wins by position →worker-firstresolvesworker,workerd-firstresolvesworkerd. This proves each condition independently rather than just "some worker file resolved." - Client selects
browser. Withworker/workerdinactive in the client build, both subpaths fall through tobrowser; the spec assertsclient-*=browserin both routers. - App Router hydration is sound.
ClientConditions.tsxstartsclient-*spans at"pending"on both server and client and only flips inuseEffect, so nosuppressHydrationWarningis needed.serverConditionscrosses the RSC boundary as a serialized prop, soserver-*spans render identically. - Pages Router hydration is sound.
suppressHydrationWarningon theclient-*spans tolerates thepending → browserpost-hydration flip; the module-leveltypeof documentread of the SSR'dserver-*spans keepsserverConditionsconsistent across hydration. The committed file usesReact.useEffect(the earlieruseLayoutEffectnit is stale). - Middleware runs for the API routes. No
matcher/configis exported, so middleware executes for/api/pages-import-conditionsand/api/app-import-conditions, injecting thex-middleware-*request headers viaNextResponse.next({ request: { headers } }). This is the same well-tested request-header-injection path used byx-from-middleware. - Server action returns RSC-env constants (
worker/workerd);useActionStatestarts at"pending"and the test asserts via auto-retryingtoHaveText. - CI wiring + real build. The spec lives under
tests/e2e/cloudflare-workers/, runs in thecloudflare-workersPlaywright project, andbeforeAlldoes a realvp build+wrangler dev, so conditions are exercised against an actual Worker bundle. - Status classification is
needs-vite-equivalent(notunsupported), consistent with siblingworker.test.ts/react-version.test.tsentries and correct given this PR adds the Worker-equivalent coverage. The unit test assertsstatus/featureviatoMatchObjectplus substringtoContainchecks, avoiding the brittle verbatim-reasonpin from an earlier revision.
Minor (non-blocking)
runtime-condition-library/package.jsondefines only the two subpathexportswith no"."/main. Harmless — nothing imports the bare specifier — but a bareimport "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.
|
Review posted to PR #2890. SummaryI reviewed the full 18-file diff and validated it locally ( No correctness bugs found. This is a well-constructed, test-only PR. Key confirmations:
One minor non-blocking note: the fixture package defines only subpath |
Why
The exact Next.js suite
test/e2e/import-conditions/import-conditions.test.tsremains 2 of 12 passing. Its other ten assertions assume the Next compiler's per-route node/edge split and privateedge-lightcondition. The vinext targeted harness intentionally uses the Node adapter, so its server paths correctly selectnode; emulating Next compiler labels there would be wrong. Cloudflare Workers instead activate the portableworkerandworkerdconditions.This keeps the exact suite in the
needs-vite-equivalentbucket and records the passing Vite-equivalent coverage against a real Wrangler Worker.Coverage
exports, matching the upstream resolver/externalization path.workerandworkerdkey order across two exports so both active conditions are independently verified.browser.Validation
node, while clientbrowserand RSCreact-serverbehavior are correct.tests/e2e/cloudflare-workers/route-handler-draft-cache.spec.tspasses 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.vp check: clean.