Remove zod inference and use explicit TypeScript types #612
+1,214
−674
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Eliminate all zod.infer usage throughout the codebase and replace with explicit TypeScript interfaces and types. Move comprehensive JSDoc documentation from zod schemas to corresponding TypeScript types. Add type assertions to ensure schema-type compatibility and update ESLint config to handle assertion types. This makes things readable by humans, and also will probably improve TS perf, and in some cases even correctness, in callers. Fixes modelcontextprotocol#119. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
@benjaminjkraft, thank you for working on this! Have you tried removing |
|
I haven't -- it seems like that might be a deeper change (e.g. affecting runtime) and have less value (i.e. the types still won't be fully readable), but I'm not sure if I really know what it would mean concretely so I may be wrong! |
|
Generally, though: I don't care if this is the approach. I care that the problem -- making this stuff readable for humans and TypeScript itself -- gets solved, and wrote the PR to show one way to solve it. If another approach can solve the problem better, I have zero attachment to the code in this PR! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #119, wherein more context on why these types are bad for users, but in short, this makes things readable by humans, and also probably improves TS perf and even in some cases correctness.
To do so, I replaced all the
zod.inferstuff, which is a mess as described in the issue, with explicit types. I moved the JSDoc to the new types, since that's where humans will read now -- and indeed they will actually be able to read! I added return types explicitly to the client, there may be other places to do this as well which I'm happy to add or can be added over time. Type assertions ensure the new types match the schemas. (There are no runtime changes.)This is a bit more annoying to maintain, of course -- the ideal would be to generate the schemas from the types and I assume there are packages to do so although I haven't explored the Zod universe lately. But in practice, Claude did all the work, so he can probably keep doing it as the types evolve.
🤖 Generated with Claude Code
Breaking Changes
This may fail users' TS builds when they upgrade, if the bad types were hiding bugs (as, in our codebase, they were). Those are already bugs for them, just ones TS couldn't catch. There are no runtime changes.
Types of changes
Checklist