feat: add React Router adapter#84
Conversation
|
@farizanjum is attempting to deploy a commit to the Dodo Payments Team on Vercel. A member of the Team first needs to authorize it. |
9e20a15 to
48f5acf
Compare
There was a problem hiding this comment.
Review: feat: add React Router adapter (@dualmark/remix)
Verdict: Approve with follow-ups. Strong, well-structured feature. Two spec-consistency nits below that I'd like to see addressed (ideally before merge, since they re-introduce bugs that sibling PRs #70/#73 are fixing) — neither blocks the core functionality.
What I exercised
Checked out the branch, ran a clean bun install, then:
@dualmark/remix: build ✓, typecheck ✓, 24/24 tests ✓examples/remix-blog: builds, generates the 4 markdown resource routes +llms.txt- Ran the production server (
react-router-serve) and drove it end-to-end with curl - Ran the CLI conformance verifier against the live server
Note: on first run test/typecheck failed with
Cannot find module @dualmark/core— that was a stale-lockfile linking artifact in my sandbox; a freshbun installfixed it. Not a PR defect.
Independent conformance result
node packages/cli/dist/cli.js verify http://127.0.0.1:3000/posts/hello
Score: 125/125 ← all 14 checks pass (incl. 406 negotiation, Link alternate, tokens, noindex, nosniff, aeoVersion)
End-to-end header checks all correct: browser → text/html + Link rel=alternate + Vary; GPTBot → text/markdown + X-Robots-Tag: noindex + X-Markdown-Tokens; Accept: text/markdown → markdown; direct .md → markdown; /llms.txt → text/plain.
Code quality — strong
config-validation.tsis thorough: rejects trailing-slash siteUrl, non-relative/..paths, and correctly rejects Windows absolute paths cross-platform viawin32.isAbsolute(the follow-up commit). Route/pattern sanitization is solid.routes.tsGENERATED_MARKERguard refusing to overwrite user-authored route files is a nice safety touch.handlers.tstargeted dispatch + resource-route factoring is clean and reuses core primitives correctly.- Intentionally rejecting
slugStrategy: "catch-all"with a clear error is the right call for the splat limitation.
Follow-up 1 (spec §5) — bot UA should respect explicit Accept: text/html
entry-server.ts:78 uses the legacy if (bot.isBot || format === "markdown"). I confirmed live: a request with User-Agent: GPTBot/1.0 and Accept: text/html is served text/markdown + noindex on the canonical URL:
curl -H 'User-Agent: GPTBot/1.0' -H 'Accept: text/html' …/posts/hello
→ content-type: text/markdown; x-robots-tag: noindex ✗ (spec §5: UA MUST NOT override explicit Accept)
PR #70 just introduced shouldServeMarkdown(accept, isBot) in @dualmark/core for exactly this. Please route this adapter through it too.
Follow-up 2 (spec §3) — Vary: Accept gated on injectLinkHeader
entry-server.ts:82-87 only calls appendVaryAccept inside if (injectLinkHeader). With injectLinkHeader: false, negotiable HTML would ship without Vary: Accept — the same cache-poisoning bug PR #73 is fixing across the other adapters. Vary: Accept should be set whenever the page is negotiable, independent of the Link flag.
Minor
- Package/PR is named "React Router" but publishes as
@dualmark/remix— intentional given RRv7 = Remix lineage, just calling it out for discoverability (README/docs do explain it).
Overall this is a high-quality adapter that already hits perfect conformance. Approving; please fold in the two @dualmark/core helpers (shouldServeMarkdown, unconditional Vary) so it lands consistent with #70/#73.
|
Thanks for the detailed review. I addressed the Vary follow-up in 3054b11: negotiable HTML now receives Vary: Accept even when injectLinkHeader is false, with a regression test covering Link absent plus Vary present. For the explicit Accept follow-up, PR #70 is still open and owns the shared shouldServeMarkdown helper. I will rebase onto it once it merges and route @dualmark/remix through that helper rather than duplicate its negotiation logic here. |
|
Thanks for the detailed review. Both follow-ups are now addressed.
Verification:
|
3054b11 to
7650514
Compare
Closes #6
Summary
@dualmark/remixfor React Router v7 Framework Mode/llms.txtfrom Dualmark configAccept: text/markdownexamples/remix-blogplus docs and release metadataTesting
bun run --filter @dualmark/remix testbun run --filter @dualmark/remix typecheckbun run --filter @dualmark/remix buildbun run --filter dualmark-example-remix-blog buildbun run --filter dualmark-docs buildnode packages/cli/dist/cli.js verify http://localhost:5174/posts/helloreturned125/125underreact-router devnode packages/cli/dist/cli.js verify http://localhost:3000/posts/helloreturned125/125underreact-router-serveNotes
slugStrategy: "catch-all"is intentionally rejected for now because React Router splats cannot safely express nested*.mdroutes without catching human HTML paths.https://reactrouter.com/start/framework/installation.