Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tweak ExportedHandler types to improve DX #14007

Merged
merged 10 commits into from
Apr 23, 2024

Conversation

Cherry
Copy link
Contributor

@Cherry Cherry commented Apr 14, 2024

Closes #14006

ExportedHandler without an explicit return type results in significantly worsened DX for users, especially when it comes to error handling. Errors are attributed the function as a whole, with IDEs marking the entire function as an issue, instead of the specific line that caused it.

Full Example

Let's take a worker from one of the examples in the docs: https://developers.cloudflare.com/workers/examples/basic-auth/

I've made one tiny edit in the worker that breaks it, but should be quick to see and resolve.

If I scroll up, you'll see the entire fetch declaration is highlighted as an error:

And the following error:

Type '(request: Request<unknown, IncomingRequestCfProperties<unknown>>, env: { PASSWORD: string; }) => Promise<false | Response>' is not assignable to type 'ExportedHandlerFetchHandler<{ PASSWORD: string; }, unknown>'.
  Type 'Promise<false | Response>' is not assignable to type 'Response | Promise<Response>'.
    Type 'Promise<false | Response>' is not assignable to type 'Promise<Response>'.
      Type 'false | Response' is not assignable to type 'Response'.
        Type 'boolean' is not assignable to type 'Response'.

So all I know, if I'm proficient enough in TypeScript to parse that error (it's pretty complex), is that somewhere in my function, I'm returning a boolean instead of a Response.


Now let's look at that, with the more traditional syntax of specifying function args:

All of a sudden, I'm now given the exact line the error occurs on, and a much simpler error to parse:
Type 'boolean' is not assignable to type 'Response'.

It's a little more verbose syntax, yes, but the DX that users get from it so significantly improved

Contrived Example Take the following Worker for example:
export interface Env {}

export default {
	async fetch(request, env, ctx) {
		return true;
	},
} satisfies ExportedHandler<Env>;

This errors with:

Type '(request: Request<unknown, IncomingRequestCfProperties<unknown>>, env: Env, ctx: ExecutionContext) => Promise<boolean>' is not assignable to type 'ExportedHandlerFetchHandler<Env, unknown>'.
  Type 'Promise<boolean>' is not assignable to type 'Response | Promise<Response>'.
    Type 'Promise<boolean>' is not assignable to type 'Promise<Response>'.
      Type 'boolean' is not assignable to type 'Response'.

Which again is a pretty complex error that requires reading through, and it ends up highlighting the fetch function in vscode as the error:

vs if you use the original setup:

export interface Env {}

export default {
	async fetch(request: Request, env: Env, ctx: ExecutionContext): Promise<Response> {
		return true;
	},
};

The error you get here is so much clearer:

Type 'boolean' is not assignable to type 'Response'.

And it highlights the actual place in the code where the issue is:

By explicitly defining a return type on handlers, users are able to get better errors. So overall, this results in slightly more verbose function annotations, but huge DX wins for shorter errors, and accurately highlighted errors to line/column numbers, which vastly outweighs the slightly additional verbosity.

See rest of thread for discussion.

content/workers/examples/cron-trigger.md Outdated Show resolved Hide resolved
content/workers/examples/cache-api.md Outdated Show resolved Hide resolved
content/workers/examples/basic-auth.md Outdated Show resolved Hide resolved
@WalshyDev
Copy link
Member

Gave my 2c on Discord already I'm happy with this change but interested to hear from @dario-piotrowicz given the initial PR that launched this.

@Cherry
Copy link
Contributor Author

Cherry commented Apr 15, 2024

Thanks @WalshyDev! I've added that, and where appropriate added a better Env declaration to these examples such as in some of the R2 ones where it was previously just typed as any.

For anyone else reading, the discussion at cloudflare/workers-sdk#5607 (comment) is a good one to look through. I strongly think the docs should be tailored towards helping new users as much as possible.

@Cherry Cherry changed the title Remove ExportedHandler types to improve DX Tweak ExportedHandler types to improve DX Apr 17, 2024
@github-actions github-actions bot added product:browser-rendering product:d1 D1: https://developers.cloudflare.com/d1/ product:durable-objects Durable Objects: https://developers.cloudflare.com/workers/learning/using-durable-objects/ product:hyperdrive Hyperdrive: https://developers.cloudflare.com/hyperdrive/ product:kv product:pages product:pub-sub Pub/Sub: https://developers.cloudflare.com/pub-sub product:queues Cloudflare Queues: https://developers.cloudflare.com/queues product:vectorize Vectorize: https://developers.cloudflare.com/vectorize/ product:workers-ai Workers AI: https://developers.cloudflare.com/workers-ai/ size/l and removed size/m labels Apr 17, 2024
@Cherry
Copy link
Contributor Author

Cherry commented Apr 17, 2024

I've updated this now to match the agreed syntax. Let me know if anyone wants any further changes!

@Cherry Cherry requested a review from pedrosousa as a code owner April 19, 2024 16:16
@kodster28
Copy link
Contributor

Checked with @penalosa, @WalshyDev, and @GregBrimble and this is good to go.

Thanks for the submission, @Cherry!

@kodster28 kodster28 merged commit 4319548 into cloudflare:production Apr 23, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product:browser-rendering product:d1 D1: https://developers.cloudflare.com/d1/ product:durable-objects Durable Objects: https://developers.cloudflare.com/workers/learning/using-durable-objects/ product:hyperdrive Hyperdrive: https://developers.cloudflare.com/hyperdrive/ product:kv product:pages product:pub-sub Pub/Sub: https://developers.cloudflare.com/pub-sub product:queues Cloudflare Queues: https://developers.cloudflare.com/queues product:r2 R2 object storage: https://developers.cloudflare.com/r2 product:vectorize Vectorize: https://developers.cloudflare.com/vectorize/ product:workers Related to Workers product product:workers-ai Workers AI: https://developers.cloudflare.com/workers-ai/ size/l
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stop using ExportedHandler for TypeScript examples