fix(core): move @cfworker/json-schema out of main barrel into /validators/cf-worker subpath#1897
fix(core): move @cfworker/json-schema out of main barrel into /validators/cf-worker subpath#1897felixweinberger wants to merge 1 commit intomainfrom
Conversation
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical re-route of an internal import path to keep the optional @cfworker/json-schema peer out of the Node bundle; public API is unchanged. Two cosmetic nits inline, neither blocking.
Extended reasoning...
Overview
This PR stops the optional-peer @cfworker/json-schema from being statically pulled into the main @modelcontextprotocol/{server,client} bundles. Mechanically it: (1) drops the wildcard re-export of cfWorkerProvider.js from the internal core/src/index.ts barrel (keeping a type-only re-export of CfWorkerSchemaDraft), (2) adds a ./validators/cfWorker subpath export to the private @modelcontextprotocol/core package, (3) repoints the five consumers (shimsWorkerd.ts ×2, shimsBrowser.ts, validators/cfWorker.ts ×2) at the new subpath, and (4) adds matching paths entries to three tsconfig.json files plus updates one integration test import.
Public API impact
None. @modelcontextprotocol/core is "private": true and is vendored into the published packages. The published public surface (packages/{server,client}/src/index.ts → core/public) already exported only the type CfWorkerSchemaDraft, never the CfWorkerJsonSchemaValidator class — that has always been reachable only via _shims conditionals or the explicit /validators/cf-worker subpath, both of which are preserved. The changeset's "No public API change" claim checks out.
Security risks
None. This is pure import-path plumbing in build/packaging config; no runtime logic, auth, validation, or protocol behaviour is touched.
Level of scrutiny
Low–medium. The fix is a clear bug (module-not-found for Node consumers without the optional peer) with an obvious, conventional solution (subpath export + code-splitting). The author verified dist/index.mjs has zero @cfworker/json-schema references and validated against 5 OSS repos. I grepped for residual imports of CfWorkerJsonSchemaValidator from the bare core barrel and found none outside the diff — the migration is complete.
Other factors
The two inline nits (dangling {@linkcode} in a non-TypeDoc-rendered JSDoc block, and the tsdown.config.ts dts.compilerOptions.paths override not mirroring the new subpath) are both cosmetic/consistency-only — the build works today via node_modules fallback and the docs build is unaffected because core is excluded from TypeDoc. Fine to address or skip.
felixweinberger
force-pushed
the
fweinberger/v2-bc-cfworker-lazy
branch
from
3b174f7 to
dd833bc
Compare
🦋 Changeset detectedLatest commit: 91c3085 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
LGTM — both prior nits addressed; the two remaining inline comments are cosmetic (changeset prose accuracy, type-import specifier hygiene) and non-blocking.
Extended reasoning...
Overview
This PR removes the export * from './validators/cfWorkerProvider.js' wildcard from the private @modelcontextprotocol/core barrel and routes all consumers (shimsWorkerd, shimsBrowser, /validators/cf-worker subpaths) through a new ./validators/cfWorker package-exports entry instead. The mechanical fan-out is: one core barrel edit, one core package.json export, three tsconfig.json path mappings, two tsdown.config.ts path mappings, five one-line import-specifier swaps in shim/re-export files, one integration-test import split, and a changeset. 14 files, but each hunk is a 1–3 line plumbing change following the same pattern.
Progress since last review
Both nits I raised on the previous revision have been addressed: the {@linkcode CfWorkerJsonSchemaValidator} reference in core/src/index.ts is now plain backticks, and both tsdown.config.ts files now include the @modelcontextprotocol/core/validators/cfWorker path mapping alongside the existing core subpaths.
Security risks
None. This is module-graph / bundler-config plumbing with no auth, crypto, network, or user-input handling. The only runtime-observable effect is which chunk @cfworker/json-schema lands in.
Level of scrutiny
Moderate-low. Build config affects published artifacts, so it's not trivial, but: (a) @modelcontextprotocol/core is "private": true so the removed barrel export was never public API — CfWorkerJsonSchemaValidator was and remains reachable only via the documented /validators/cf-worker subpath and the _shims conditional in the published server/client packages; (b) the PR author verified dist/index.mjs has zero @cfworker references post-change and ran the full typecheck/lint/test matrix plus the v2-bc OSS integration suite; (c) CfWorkerSchemaDraft is still type-re-exported from the core barrel so downstream type references don't break.
Other factors
The two new inline nits (changeset overstates the failure mode as "module-not-found" when the dep was actually inlined; CfWorkerSchemaDraft type re-export could use the same subpath specifier as the value export) are both explicitly cosmetic with zero runtime or published-API impact. They don't meet the bar to block a packaging-hygiene fix that's otherwise correct and already responsive to review.
CfWorkerJsonSchemaValidator was re-exported from the core internal barrel, pulling the optional-peer @cfworker/json-schema static import into every Node consumer's bundle. Move it to a dedicated core/validators/cfWorker subpath; the workerd/browser _shims and the public /validators/cf-worker subpath import from there instead. Main index.mjs no longer references the package.
felixweinberger
force-pushed
the
fweinberger/v2-bc-cfworker-lazy
branch
from
dd833bc to
91c3085
Compare
There was a problem hiding this comment.
LGTM — all four nits from both review rounds are addressed; the changeset now accurately describes the inlining fix and the subpath wiring is consistent across tsconfig/tsdown.
Extended reasoning...
Overview
This PR stops the @cfworker/json-schema dev dependency from being inlined into the main dist/index.mjs of the published @modelcontextprotocol/{server,client} packages. It does so by removing the export * from './validators/cfWorkerProvider.js' from the (private) core barrel and routing all consumers — the workerd/browser _shims files and the public /validators/cf-worker subpath re-exports — through a new @modelcontextprotocol/core/validators/cfWorker subpath instead. Supporting changes: a new exports entry in packages/core/package.json, matching path mappings in three tsconfig.json files and both tsdown.config.ts DTS overrides, one integration-test import update, and a changeset.
I verified that the published packages' main barrels (packages/{server,client}/src/index.ts) re-export from @modelcontextprotocol/core/public, not from the core root barrel, and that core/public only ever exported CfWorkerSchemaDraft as a type (line 141) — so dropping the value export from the core root barrel does not change the public API surface of the shipped packages. The "No public API change" claim in the changeset is accurate.
Security risks
None. This is purely module-graph / bundler-chunking work with no runtime logic, auth, input handling, or network changes.
Level of scrutiny
Low–moderate. The blast radius is build/packaging, where mistakes are loud (typecheck/build failures) and CI is green. The change is mechanical: every edited line is either an import-specifier swap or a path-mapping addition that mirrors the existing ./public pattern one-for-one. The only file with non-trivial reasoning is core/src/index.ts, and the in-source comment there explains the intent clearly.
Other factors
- All four of my prior nits (JSDoc
@linkcode, tsdowndts.compilerOptions.pathsparity, changeset wording accuracy, and using the subpath for theCfWorkerSchemaDrafttype re-export) are addressed in the current diff. - The integration test in
elicitation.test.tscontinues to exerciseCfWorkerJsonSchemaValidatorend-to-end via the new import path. - Part of the documented
v2-bcseries; scope is tightly bounded to the one concern in the title.
1 participant
Part of the v2 backwards-compatibility series — see reviewer guide.
The CF-Worker validator was statically imported into the main barrel, crashing Node/browser/bun consumers without the optional
@cfworker/json-schemadep. This moves it to a dedicated subpath so tsdown code-splits it.Motivation and Context
The CF-Worker validator was statically imported into the main barrel, crashing Node/browser/bun consumers without the optional
@cfworker/json-schemadep. This moves it to a dedicated subpath so tsdown code-splits it.v1 vs v2 pattern & evidence
v1 pattern:
v2-native:
SameEvidence: Verified
packages/{server,client}/dist/index.mjshave 0@cfworker/json-schemareferences after this change.How Has This Been Tested?
v2-bc-integrationvalidation branchpnpm typecheck:all && pnpm lint:all && pnpm test:allgreenBreaking Changes
None — additive
@deprecatedshim. Removed in v3.Types of changes
Checklist
Additional context
Stacks on: none