Skip to content

fix(openapi): keep server code out of the OpenAPI Link entry - #2358

Open
florian-lefebvre wants to merge 7 commits into
middleapi:mainfrom
florian-lefebvre:florian-lefebvre/sunfish
Open

florian-lefebvre wants to merge 7 commits into
middleapi:mainfrom
florian-lefebvre:florian-lefebvre/sunfish

Conversation

@florian-lefebvre

@florian-lefebvre florian-lefebvre commented Oct 29, 2026 •

Copy link
Copy Markdown

Bundlers that tree-shake, such as esbuild, webpack, and Rspack, keep the OpenAPI Handler out of client bundles that import from @orpc/openapi/fetch. If your bundler cannot tree-shake, such as Metro, import OpenAPILink from @orpc/openapi/fetch/link instead. That entry contains only the link, so server code stays out of the bundle:

import { OpenAPILink } from '@orpc/openapi/fetch/link'

AI was used to prepare this change.

OpenAPILink moves to @orpc/openapi/fetch/link so Metro stops including the OpenAPI handler, @orpc/server, and rou3 in client bundles. @orpc/openapi/fetch keeps OpenAPIHandler.

The link imports OpenAPILinkCodec directly, serializeHeaders moves to its own module, and Lazy, Lazyable, and unlazy move to @orpc/contract.
@florian-lefebvre
florian-lefebvre marked this pull request as ready for review October 29, 2026 15:56

@pullfrog pullfrog 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.

Important

The @orpc/server re-export silently drops the LazyDefinition public type, which the refactor was otherwise meant to preserve.

Reviewed changes

  • OpenAPILink moved to a client-only entry — @orpc/openapi/fetch now exports only OpenAPIHandler; the link lives at @orpc/openapi/fetch/link (added to both dev and publishConfig exports). Verified via pnpm --filter @orpc/openapi prepack: dist/adapters/fetch/link.mjs imports only @orpc/client/*, @orpc/contract, @orpc/shared, and @standard-server/{core,fetch} — no @orpc/server, no rou3.
  • Server code kept out of the link graph — the link imports OpenAPILinkCodec directly instead of the ../standard barrel, serializeHeaders moved from standard/utils.ts to standard/serialize-headers.ts, and the handler adapters import openapi-handler-codec directly. New fetch/link.test.ts guards all of this by vi.mock-ing the server modules and rou3 with throwing factories.
  • Lazy/Lazyable/unlazy relocated to @orpc/contract — @orpc/contract's index now star-exports ./lazy, and packages/server/src/lazy.ts re-exports from @orpc/contract; openapi/src/meta.ts and the link codec now source Lazy/unlazy from @orpc/contract.
  • Docs, skills, playgrounds, and tests updated to the new import path; no remaining OpenAPILink import from @orpc/openapi/fetch in tracked files.

Ran pnpm vitest run on the fetch/standard/contract-lazy tests (23 pass) and pnpm --filter @orpc/contract --filter @orpc/openapi type:check (passes).

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Comment thread packages/server/src/lazy.ts Resolved
Restore LazyDefinition on the @orpc/server public surface after the lazy module moved to @orpc/contract.

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes

The only change since the prior pullfrog review is a one-line type re-export, which resolves the outstanding finding.

  • Restored LazyDefinition on @orpc/server — packages/server/src/lazy.ts now re-exports Lazyable and LazyDefinition from @orpc/contract, closing the type-only regression the previous review flagged.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@dinwwwh

dinwwwh commented Oct 30, 2026

Copy link
Copy Markdown
Member

Thanks for tracking this down! Could we make this non-breaking? Keep OpenAPILink exported from @orpc/openapi/fetch and add /fetch/link alongside it.

Bundlers with tree-shaking (esbuild, webpack, Rspack) already drop the handler from /fetch. Only Metro doesn't: 163 KB with /fetch vs 82 KB with your /fetch/link. Expo's experimental tree-shaking doesn't help, because it skips names exported via export { ... } lists, which is what our build emits. So React Native apps can use a tree-shaking bundler like Re.Pack, or import from /fetch/link on Metro.

Changes:

  • Restore export * from './openapi-link' in fetch/index.ts and revert fetch/index.test.ts.
  • Keep /fetch/link, link.test.ts, and the three internal changes (direct codec import, serialize-headers.ts, Lazy in @orpc/contract).
  • Keep /fetch as the documented import, and add a note on the Link page: if your bundler doesn't tree-shake (e.g. Metro), use /fetch/link.
  • Drop the ! from the title.

@orpc/openapi/fetch exports OpenAPILink again, so the existing import path keeps working. The client-only @orpc/openapi/fetch/link entry stays for bundlers such as Metro that cannot tree-shake the handler out of the fetch entry. Docs keep /fetch as the documented import and point to /fetch/link for those bundlers.
@florian-lefebvre florian-lefebvre changed the title fix(openapi)!: keep server code out of the OpenAPI Link entry fix(openapi): keep server code out of the OpenAPI Link entry Oct 30, 2026
…-shake

The Link page keeps @orpc/openapi/fetch as the documented import and points Metro readers to the client-only @orpc/openapi/fetch/link entry in paragraph text.
The link entry fix does not need the serializeHeaders module rename or deep imports of the handler codec in the server adapters. Restore utils.ts and keep the ../standard imports in the handler adapters, so the diff only contains changes the fix requires.
@florian-lefebvre

Copy link
Copy Markdown
Author

@dinwwwh thanks for the review, done!

@pullfrog pullfrog 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.

✅ No new issues found. The reverted scope is sound and the link entry is verifiably server-free.

Reviewed changes

Incremental delta since the prior pullfrog review (523140f → 1a410c9): the PR was reduced from a breaking entry move to a non-breaking, additive change.

  • Restored OpenAPILink on @orpc/openapi/fetch — packages/openapi/src/adapters/fetch/index.ts again exports both OpenAPIHandler and OpenAPILink, so existing imports keep working; @orpc/openapi/fetch/link remains the client-only entry.
  • Dropped the unrelated churn — the handler-adapter import rewrites and the standard/utils.ts → serialize-headers.ts split were reverted; serializeHeaders again lives in standard/utils.ts, and no serialize-headers references remain anywhere.
  • Docs narrowed to the new guidance — only openapi/link.mdx changed: the default import stays @orpc/openapi/fetch, with /link recommended for bundlers that do not tree-shake (e.g. Metro). All other docs still import from @orpc/openapi/fetch, consistent with the additive approach.
  • Lazy relocation kept — Lazy/unlazy/Lazyable/LazyDefinition still live in @orpc/contract and are re-exported from @orpc/server; the openapi-link-codec.ts unlazy import still points at @orpc/contract, which is what keeps server code out of the link graph.

Verified with pnpm --filter @orpc/openapi prepack: dist/adapters/fetch/link.mjs and every shared chunk it imports reference only @orpc/client, @orpc/contract, @orpc/shared, and @standard-server/* — no @orpc/server, no rou3 (the one shared chunk that does reference them, openapi.NQL3UlBV.mjs, is not in the link graph). Targeted vitest run on the changed suites (6 pass) and type:check for @orpc/contract/@orpc/openapi also pass.

ℹ️ PR description still announces a breaking change

The body says "This is a breaking import change for link users" and shows a @orpc/openapi/fetch → @orpc/openapi/fetch/link migration diff, but 522804d24 restored the OpenAPILink export on the main entry, so the change is now additive. Worth updating the description so reviewers and any release-note tooling do not advertise a breaking migration that no longer exists.

Technical details
# Stale breaking-change claim in PR description

## Affected sites
- PR #2358 description — "This is a breaking import change for link users." plus the diff block showing `-import { OpenAPILink } from '@orpc/openapi/fetch'`.

## Required outcome
- The description reflects the current additive approach: `OpenAPILink` stays on `@orpc/openapi/fetch`; `/link` is only recommended for bundlers that do not tree-shake.

## Suggested approach (optional)
- Note that the `!`-marked commit `cba48c46` was superseded by `522804d24`, and that the PR title (used for the squash commit) carries no breaking marker.

</details>

<!--
Pullfrog review metadata. These findings were written against 1a410c9;
if commits have landed on florian-lefebvre/sunfish since, treat every specific bug, file, or
line callout as POTENTIALLY STALE and re-diff before acting on it.

- Mode: IncrementalReview (delta against prior pullfrog review)
- Files reviewed: 11
- Commits reviewed: 6
- Base: main (49751e2)
- Head: florian-lefebvre/sunfish (1a410c9)
- Reviewed commits:
  - 1a410c97 — Merge branch 'main' into florian-lefebvre/sunfish
  - bfcaffc3 — refactor(openapi): drop unrelated churn from the link entry fix
  - 35a9822c — docs(openapi): document the link import for bundlers that do not tree-shake
  - 522804d2 — fix(openapi): keep OpenAPILink exported from the fetch entry
  - 523140fa — fix(server): re-export LazyDefinition type
  - cba48c46 — fix(openapi)!: keep server code out of the OpenAPI Link entry
- Prior pullfrog review: 523140f (https://github.com/middleapi/orpc/pull/2358#pullrequestreview-5701071610)
-->


<!-- PULLFROG_DIVIDER_DO_NOT_REMOVE_PLZ -->
<sup><a href="https://pullfrog.com"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pullfrog.com/logos/frog-white-full-18px.png"><img src="https://pullfrog.com/logos/frog-green-full-18px.png" width="9px" height="9px" style="vertical-align: middle; " alt="Pullfrog"></picture></a>&nbsp;&nbsp;| [View workflow run](https://github.com/middleapi/orpc/actions/runs/41020258770/job/123945726977) | Using `deepseek-v4.1-flash` (free via [Pullfrog for OSS](https://pullfrog.com/for-oss)) | [𝕏](https://x.com/pullfrogai)</sup>

@florian-lefebvre

Copy link
Copy Markdown
Author

I updated it

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
orpc 9f214d8 Commit Preview URL

Branch Preview URL
Oct 30 2026, 08:10 AM

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes

The delta since the prior pullfrog review (1a410c9 → 9f214d8) is a cleanup of the Lazy relocation plus a docs reorganization; no behavioral regressions found.

  • Removed the @orpc/server lazy shim — deleted packages/server/src/lazy.ts and pointed every server module's Lazy/unlazy/Lazyable import at @orpc/contract directly.
  • Preserved @orpc/server's public surface — index.ts now re-exports Lazy/unlazy (value block) and Lazyable/LazyDefinition (type block) from @orpc/contract, exactly what the shim forwarded before.
  • Fixed the contract self-import — packages/contract/src/lazy.ts sources Meta/AnyMetaPlugin from ./meta instead of self-referencing @orpc/contract.
  • Relocated the /link tip — moved out of openapi/link.mdx into openapi/adapters.mdx and adapters/fetch-api.mdx.
  • Simplified the lazy instanceof assertion — builder.test.ts now asserts against ContractModule.Lazy (a single class instance).

Verified the link entry is still server-free: pnpm --filter @orpc/openapi prepack plus a full import-graph walk from dist/adapters/fetch/link.mjs reach only @orpc/client, @orpc/contract, @orpc/shared, and @standard-server/* — no @orpc/server, no rou3. type:check for @orpc/{server,contract,openapi}, targeted vitest run (48 pass), and eslint on the changed files all pass.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@dinwwwh

dinwwwh commented Oct 30, 2026

Copy link
Copy Markdown
Member

Thanks for the changes, and sorry for the back-and-forth. I'd rather not add a separate /fetch/link entry. I'll improve tree-shaking in our build instead, so Expo can drop the handler from @orpc/openapi/fetch.

Once that's released, enable tree-shaking in your app:

EXPO_UNSTABLE_TREE_SHAKING=1
EXPO_UNSTABLE_METRO_OPTIMIZE_GRAPH=1

@florian-lefebvre

Copy link
Copy Markdown
Author

No problem, I also prefer that solution thanks! I already have treeshaking enabled. I'll let you close the PR

This branch has not been deployed

No deployments
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.

2 participants