Skip to content

Conversation

@NejcZdovc
Copy link

@NejcZdovc NejcZdovc commented Dec 1, 2025

Resolving #5617

More info with detailed description in #5617

@NejcZdovc NejcZdovc requested review from a team as code owners December 1, 2025 15:58
@github-actions
Copy link

github-actions bot commented Dec 1, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@NejcZdovc
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Dec 1, 2025
@danlapid
Copy link
Collaborator

danlapid commented Dec 1, 2025

@penalosa

@penalosa
Copy link
Contributor

penalosa commented Dec 1, 2025

I don't think this name is correct. As I understand it this PR is adding the option to drill the type of props through ExportedHandler? If so, the type parameter should be called Props

@NejcZdovc
Copy link
Author

@penalosa should I call it ExecutionContextProps?

@penalosa
Copy link
Contributor

penalosa commented Dec 1, 2025

@NejcZdovc I think it should just be called Props

@NejcZdovc
Copy link
Author

@penalosa changed it to Props

@kentonv
Copy link
Member

kentonv commented Dec 1, 2025

It should just be called Props. That's what it's called e.g. on the WorkerEntrypoint class and all the other places it appears -- it's not specific to ExecutionContext.

I'm a bit concerned that we're adding this as the fourth parameter to ExportedHandler when the second and third parameters are things that people are much less likely to want to customize. QueueHandlerMessage in particular is rarely overridden. I wonder if we should rethink this type signature entirely, maybe behind a compat flag.

@penalosa What do you think?

@penalosa
Copy link
Contributor

penalosa commented Dec 1, 2025

@kentonv I agree, I think we should make this match WorkerEntrypoint (Env, then Props) behind a compat flag. It's probably worth getting this in as-is and then following up with that though, so that people on an older compat date are at least able to set the type of props (even if it's unergonomic)?

@kentonv
Copy link
Member

kentonv commented Dec 1, 2025

Well, the reason I figured I could skip this entirely is because people using ctx.props should almost always be using WorkerEntrypoint, not ExportedHandler.

I would like to understand the use case here better -- I asked a question on the issue #5617.

Copy link
Contributor

@jamesopstad jamesopstad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. I can approve this once the email handler types have also been updated. I'll also make a note that we should consider changing the type parameter order in future.

);
JSG_STRUCT_TS_OVERRIDE(<Env = unknown, QueueHandlerMessage = unknown, CfHostMetadata = unknown> {
JSG_STRUCT_TS_OVERRIDE(<Env = unknown, QueueHandlerMessage = unknown, CfHostMetadata = unknown, Props = unknown> {
email?: EmailExportedHandler<Env>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EmailExportedHandler also needs updating here:

declare type EmailExportedHandler<Env = unknown> = (
message: ForwardableEmailMessage,
env: Env,
ctx: ExecutionContext
) => void | Promise<void>;

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.

5 participants