feat: add Props type parameter to ExportedHandler for typed ctx.props #784
Conversation
Add a Props type parameter to ExportedHandler so that ctx.props can be properly typed when using `satisfies ExportedHandler<Env, Props>`. - Add enhanced ExportedHandler<Env, Props, QueueHandlerMessage, CfHostMetadata> - Add Props-aware handler types (ExportedHandlerFetchHandler, etc.) - Export types from main entry point and agents/types - Add generic params to createMcpHandler and McpAgent.serve() Fixes cloudflare#501
🦋 Changeset detectedLatest commit: 6b84f4f The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
whoiskatrin
requested review from
deathbyknowledge,
mattzcarey and
threepointone
commit: |
There was a problem hiding this comment.
I'm not sure we should be typing these here. These are already defined in the output of wrangler types if I am not mistaken.
There was a problem hiding this comment.
Thanks for the quick reply! I want to make sure I understand the concern. Are you saying wrangler types already generates ExportedHandler with a Props parameter? I added Props support to enable typed ctx.props which I don't believe exists in the current wrangler types output. Should these types live elsewhere, or should I be extending the wrangler generated types differently?
There was a problem hiding this comment.
I believe what @mattzcarey meant was that the changes to add the Props parameter was better fit for workerd repo which is where these types come from, instead of duplicating them here.
I've created a PR already there with the changes cloudflare/workerd#5989 so the new types are not needed
| } | ||
|
|
||
| export function createMcpHandler( | ||
| export function createMcpHandler< |
There was a problem hiding this comment.
this seems like a good improvement independently of the types below.
|
I'll need to review this |
deathbyknowledge
left a comment
deathbyknowledge
left a comment
There was a problem hiding this comment.
Let's keep the changes in in /mcp but remove the rest
packages/agents/src/mcp/handler.ts
Outdated
| export function createMcpHandler( | ||
| export function createMcpHandler< | ||
| Env = unknown, | ||
| Props = unknown |
There was a problem hiding this comment.
| Props = unknown | |
| Props extends Record<string, unknown> = Record<string, unknown> |
packages/agents/src/mcp/handler.ts
Outdated
|
|
||
| export function createMcpHandler( | ||
| export function createMcpHandler< | ||
| Env = unknown, |
There was a problem hiding this comment.
| Env = unknown, | |
| Env extends Cloudflare.Env = Cloudflare.Env |
packages/agents/src/mcp/handler.ts
Outdated
| */ | ||
| export function experimental_createMcpHandler( | ||
| export function experimental_createMcpHandler< | ||
| Env = unknown, |
packages/agents/src/mcp/index.ts
Outdated
| * Defaults to Streamable HTTP transport. | ||
| */ | ||
| static serve( | ||
| static serve<Env = unknown, Props = unknown>( |
There was a problem hiding this comment.
I believe what @mattzcarey meant was that the changes to add the Props parameter was better fit for workerd repo which is where these types come from, instead of duplicating them here.
I've created a PR already there with the changes cloudflare/workerd#5989 so the new types are not needed
|
Thanks for the review! I understand now that I was unnecessarily re inventing types that already exist. Will fix this later today. Really appreciate the feedback |
- Update MCP handler type constraints per reviewer suggestion: - Env extends Cloudflare.Env = Cloudflare.Env - Props extends Record<string, unknown> = Record<string, unknown> - Remove ExportedHandler types (will be added to workerd repo instead) - Update tests to work with new type constraints - Update changeset description
4 participants
Add a Props type parameter to ExportedHandler so that ctx.props can be properly typed when using
satisfies ExportedHandler<Env, Props>.Fixes #501