[C3] Use satisfies ExportedHandler for handlers in templates

[morinokami]

What this PR solves / how to test

Use satisfies ExportedHandler for handlers in templates. This will improve autocompletion in the editor and make the code cleaner.

Author has addressed the following

• Tests
  • TODO (before merge)
  • Included
  • Not necessary because: no functional changes
• Changeset (Changeset guidelines)
  • TODO (before merge)
  • Included
  • Not necessary because: no functional changes
• Public documentation
  • TODO (before merge)
  • Cloudflare docs PR(s): https://github.com/cloudflare/cloudflare-docs/pull/...
  • Not necessary because: no functional changes

[changeset-bot]

🦋 Changeset detected

Latest commit: c451235

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
create-cloudflare	Patch

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

[github-actions]

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9472778657/npm-package-wrangler-5607

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5607/npm-package-wrangler-5607

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9472778657/npm-package-wrangler-5607 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9472778657/npm-package-create-cloudflare-5607 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9472778657/npm-package-cloudflare-kv-asset-handler-5607

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9472778657/npm-package-miniflare-5607

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9472778657/npm-package-cloudflare-pages-shared-5607

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9472778657/npm-package-cloudflare-vitest-pool-workers-5607

Note that these links will no longer work once the GitHub Actions artifact expires.

---

wrangler@3.60.2 includes the following runtime dependencies:

Package	Constraint	Resolved
miniflare	workspace:*	3.20240605.0
workerd	1.20240605.0	1.20240605.0
workerd --version	1.20240605.0	2024-06-05

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

[dario-piotrowicz]

Nice improvement! I really like it 😄

Thanks a bunch @morinokami ❤️

Could you please create a changeset as well for this change? (I'd classify this as a feature I guess 🤔)

[dario-piotrowicz]

@morinokami thanks for the changeset 😄

Again LGTM 😃

[Cherry]

I think this actually results in worsened DX sadly.

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 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:

This is obviously a contrived example, but for more complex workers, with hundreds of line, isolating errors is now significantly more complex, especially for those newer to TypeScript.

Here's a more detailed example: cloudflare/cloudflare-docs#14006

[dario-piotrowicz]

@Cherry thanks a bunch for the comment 🙂

I can see your point, let's see what the rest of the team thinks 👍

Especially I'd be keen to see the response to cloudflare/cloudflare-docs#14007 and follow whatever the consensus there is (move await from or stick with satisfies ExportedHandler)

[morinokami]

@Cherry Thank you for your comment! Indeed, particularly for newcomers to TypeScript, it might be easier to understand errors if the return statement line is highlighted directly, while the satisfies ExportedHandler error messages may seem a bit difficult to read.

However, it is also true that specifying the type offers various advantages. Here are some benefits worth considering:

• It's immediately clear what handlers can be used with a Worker. This is particularly beneficial for those new to Workers, providing them with an opportunity to learn about the overall system through its type.
• Autocompletion aids significantly during coding. For instance, typing queue( in VS Code prompts the display of arguments and other relevant information. Clearly, this is crucial for improving the Developer Experience.
  • Conversely, without type information, you must constantly reference documentation, which significantly worsens DX.
  • Moreover, manually specifying types risks incorrect type assignments. Errors in type might not be detected during static analysis and only become apparent at runtime, which severely impacts DX.
• Should the specifications for Workers change, developers will be promptly informed if the ExportedHandler type information is updated accordingly. Without specifying the type, manual adjustments might fail to capture necessary changes.

Due to these advantages of specifying types, some developers, including myself, prefer to use constructs like satisfies ExportedHandler. Requiring them to type this on every project creation might also be seen as a drawback.

I believe it's important to consider all of these factors collectively. Again, thank you for your valuable feedback 👍

[Cherry]

Thank you for the well thought out response @morinokami!

It's immediately clear what handlers can be used with a Worker. This is particularly beneficial for those new to Workers, providing them with an opportunity to learn about the overall system through its type.

While I can see the benefit here, this isn't always true. ExportedHandler is not an automated type like many others in workers-types, and is written and maintained by hand. It has been out of sync many times for months until a community member updates it, such as cloudflare/workerd#681. If this type was automated and reliable, I could get behind this a little more.

Autocompletion aids significantly during coding. For instance, typing queue( in VS Code prompts the display of arguments and other relevant information. Clearly, this is crucial for improving the Developer Experience.

This is definitely nice, but I would argue is not the most common use-case. If you're working with a brand new product like Queues, or Email Workers. it's pretty much a guarantee you are going to have to check the docs for what to add to your wrangler.toml, or tweak in your Cloudflare dashboard, or just understand how to use the product. I think this precise point could be solved with code-mods from C3 in future, but alone not a good reason to worsen DX in fetch etc. handlers.

Should the specifications for Workers change, developers will be promptly informed if the ExportedHandler type information is updated accordingly. Without specifying the type, manual adjustments might fail to capture necessary changes.

This is valid, but simply is not going to happen with Workers due to their compatibility guarantees. If this did ever happen, it would be behind a compatibility date as per the backwards compat model in workers which would result in there being multiple different ExportedHandlers depending on which compat date version you were using.

Due to these advantages of specifying types, some developers, including myself, prefer to use constructs like satisfies ExportedHandler. Requiring them to type this on every project creation might also be seen as a drawback.

I strongly believe this is a power user mentality, which is valid, but not what I'd consider C3 to be targeting with their templates.

In conclusion, while some of the points you have made are valid in isolation, I do not believe they trump the serious developer experience issues that are present with handling errors, a much more common scenario, for new users to the platform.

[morinokami]

@Cherry Even if the ExportedHandler type isn't perfect currently, it's certainly better than having no type at all. Personally, I think that using the type effectively for now, despite its flaws, and then exploring policies such as automating type updates can lead to much better DX ultimately.

However, I do acknowledge that there are several valid points in your perspective. I think this issue doesn't have a clear right or wrong answer; rather, the right approach may vary based on one's experience and perspective.

My hope is that the final decision delivers the greatest benefit to developers. The maintainers are likely more familiar with what Cloudflare users need, and as @dario-piotrowicz suggests, I believe that they should be the ones to make the final decision.

I appreciate this meaningful discussion, and thank you again!

[Cherry]

Even if the ExportedHandler type isn't perfect currently, it's certainly better than having no type at all. Personally, I think that using the type effectively for now, despite its flaws, and then exploring policies such as automating type updates can lead to much better DX ultimately.

Even if/once automated, I'm not aware of any way with this typing approach to get meaningful and useful errors for line numbers within the fetch handler. I do not believe better DX can be ultimately argued here all the while that the lack of useful error annotations remains true.

However, I do acknowledge that there are several valid points in your perspective. I think this issue doesn't have a clear right or wrong answer; rather, the right approach may vary based on one's experience and perspective.

I absolutely agree here. My personal opinion is that C3 targets newer users (to both Cloudflare, and maybe even JS/TS) way more than power users, and optimising for them should be the priority. Helpful and meaningful errors will be a huge thing for these folks.

For advanced use-cases, or when you're intimately familiar with Workers and your fetch handlers look basically just like a hono router handler or something alike, ExportedHandler can be very nice. But I don't think that's the the common use-case C3 should be optimising for, and it's especially not the common use-case throughout the cloudflare-docs repo, where hundred+ line fetch handlers are common.

I too appreciate the meaningful discussion, and look forward to what teams at Cloudflare decide is the best approach here.

[dario-piotrowicz]

@morinokami Thanks again for the PR 🙂

Although I can see the benefit in your approach after better understanding the various tradeoffs I'm also leaning towards @Cherry's version

Although less robust, it is likely much more friendly for newcomers, more advanced users should be able to switch easily to the satisfies ExportedHandler version if they prefer it (I wish we could just make the type more discoverable 🤔)

[DaniFoldi]

@Cherry will not like me for this, and I know his opinion about the explicit type cast syntax and satisfies for workers in general, so why not use both?

See my screenshot attached to see what gets underlined. In case 3 (where both are annotated) get the DX of the return false line getting underlined and the error shown is the same as case 2, and we get the future-proof syntax should a user wish to add queue/tail/email/... to their worker.

[morinokami]

@dario-piotrowicz Thank you for your comment!

Although less robust, it is likely much more friendly for newcomers, more advanced users should be able to switch easily to the satisfies ExportedHandler version if they prefer it (I wish we could just make the type more discoverable 🤔)

Yeah, I think it would be problematic for those who want to use this type if it were to be completely removed from the Docs code examples. Currently, it seems that there is very little information about the type in the Docs (as shown in the screenshot below, there is only one search result, and it does not even describe the ExportedHandler). It would be very helpful if the documentation could include information about this type in a way that is easy for such users to find.

[morinokami]

@DaniFoldi Thanks for your feedback! I really like your idea. Indeed, it seems that test3 would satisfy both Cherry's and my requirements to some extent. The error message is easy to understand, and we also benefit from the typing. The only issue might be that it looks a bit complicated and verbose. I'd like to hear @Cherry's opinion on this.

[Cherry]

With something like the following, the errors are indeed handled in a way that's helpful for new users:

export interface Env {}

export default {
	async fetch(request: Request, env: Env, ctx: ExecutionContext): Promise<Response> {
		const something = false;
		const somethingElse = true;
		if(something){
			return 'nope';
		}
		if(!somethingElse){
			return 'nope';
		}
		return new Response('Hello, world!')
	},
} satisfies ExportedHandler<Env>;

The only drawbacks to this approach are the duplicate nature now in userland typings, and ensuring you match up Env in multiple places or you get annoying errors like

Type '(request: Request<unknown, CfProperties<unknown>>, env: Env, ctx: ExecutionContext) => Promise<Response>' is not assignable to type 'ExportedHandlerFetchHandler<unknown, unknown>'.
  Types of parameters 'env' and 'env' are incompatible.
    Type 'unknown' is not assignable to type 'Env'.

If this is the approach folks want to take, I'd be okay with it, and it seems like a reasonable compromise. It makes error output mostly useful (outside of mismatched typings), and helps with the previous concerns around autocomplete for additional handlers like scheduled, etc.

[penalosa]

See cloudflare/cloudflare-docs#14007 (comment), but TLDR I think we should use satisfies ExportedHandler, but also include an explicit return type annotation of Promise<Response> on fetch handlers to improve the error location DX

[morinokami]

@penalosa Thank you! I've updated the code to match the agreed-upon style in cloudflare/cloudflare-docs#14007. @Cherry did the same thing there, so the docs and c3 are now in sync 😁

[Cherry]

This looks great and would be awesome to land soon so future projects scaffolded with C3 get the best experience.

[petebacondarwin]

I have run the C3 tests locally and they pass so I'm going to merge this PR. Thanks for the work and the discussion everyone!

[holopin-bot]

Looks like @workers-devprod is trying to award a holobyte, but something went wrong while doing so. See this page for more information.