feat(core): add deprecate() warn-once helper for v1-compat shims#1899
feat(core): add deprecate() warn-once helper for v1-compat shims#1899felixweinberger wants to merge 1 commit intomainfrom
Conversation
🦋 Changeset detectedLatest commit: a33c305 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
@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 — tiny internal helper with test coverage; the inline nit is cosmetic only.
Extended reasoning...
Overview
Adds a 22-line deprecate(key, msg) warn-once helper in packages/core/src/util/deprecate.ts (module-level Set for dedup, console.warn output), a test-only _resetDeprecationWarnings(), a vitest covering warn-once semantics, a changeset, and a one-line export from the core barrel. This is the base PR of the v2-bc shim series.
Security risks
None. It's a stateless wrapper around console.warn with no I/O, no user input handling, and no auth/transport interaction.
Level of scrutiny
Low. Per CLAUDE.md § Public API Exports, packages/core/src/index.ts is the internal barrel (private: true, consumed only by sibling packages), so exporting deprecate here does not expand the end-user public API — it just makes the helper available to the server/client packages that will host the actual shims. The function is also tagged @internal. The repo guideline that abstractions need a concrete callsite is satisfied in spirit by the v2-bc series decomposition (principle 6: decompose by default), and the changeset accurately describes it as internal.
Other factors
Logic is trivially correct and matches the test. _resetDeprecationWarnings is intentionally not re-exported from the barrel. The one inline nit (runtime export placed under the "Core types only" comment) is purely organizational and has zero functional impact — fine to fix in a follow-up or when the next stacked PR touches that file.
|
|
||
| // Core types only - implementations are exported via separate entry points | ||
| export type { JsonSchemaType, JsonSchemaValidator, jsonSchemaValidator, JsonSchemaValidatorResult } from './validators/types.js'; | ||
| export { deprecate } from './util/deprecate.js'; |
There was a problem hiding this comment.
🟡 Nit: this runtime export lands directly under the // Core types only - implementations are exported via separate entry points comment, which makes that section header read inaccurately. Consider moving it up next to the other ./util/* runtime exports (inMemory, schema, standardSchema) around lines 15-17.
Extended reasoning...
What's off
packages/core/src/index.ts is organized into loose sections. Lines 15-17 group all the runtime re-exports from ./util/*:
export * from './util/inMemory.js';
export * from './util/schema.js';
export * from './util/standardSchema.js';Further down, the file ends with a section explicitly labeled as type-only:
// Core types only - implementations are exported via separate entry points
export type { JsonSchemaType, JsonSchemaValidator, ... } from './validators/types.js';
export { deprecate } from './util/deprecate.js'; // ← new lineThe new deprecate export is a runtime value, not a type, so appending it immediately after the "Core types only" comment contradicts that comment and breaks the existing grouping convention.
Why it matters (mildly)
There is zero functional impact — the symbol is exported correctly either way. The only cost is readability: a future reader scanning the barrel will see a section comment that no longer describes what follows it, and the ./util/* exports are now split across two non-adjacent locations in the file.
Step-by-step
- Reader opens
index.tslooking for util exports → finds the block at lines 15-17. deprecateisn't there; it's 35 lines lower, under a comment that says "types only".- The comment now misdescribes its section.
Fix
Move the line up to sit with its siblings:
export * from './util/inMemory.js';
export * from './util/schema.js';
export * from './util/standardSchema.js';
export { deprecate } from './util/deprecate.js';One-line move, purely cosmetic.
|
Closing — the deprecate() runtime helper is no longer used. Per review, BC shims use @deprecated JSDoc only (matching v1's state); no runtime warnings. The C-track PRs that previously stacked on this are now rebased directly onto main. |
1 participant
Part of the v2 backwards-compatibility series — see reviewer guide.
Shared helper that emits one stderr deprecation warning per key per process. Used by all Layer-1 BC shims so hot paths (per-tool registration) don't spam.
Motivation and Context
Shared helper that emits one stderr deprecation warning per key per process. Used by all Layer-1 BC shims so hot paths (per-tool registration) don't spam.
v1 vs v2 pattern & evidence
v1 pattern:
v2-native:
Evidence: n/a
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