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

Remove Fetcher methods get(), put(), delete() with compat flag. #1770

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

kentonv
Copy link
Member

@kentonv kentonv commented Mar 5, 2024

These were intended to be convenient wrappers around fetch() that perform the respective HTTP method. It was a bad idea (my fault). Luckily it was never documented and probably few people depend on it.

Note that a long time ago, KV namespace bindings were Fetchers rather than being the specific KvNamespace type. This had lots of problems, but sort of worked as long as your key names were URL-safe. It's possible very old scripts are in production that still depend on these old-style bindings, but they won't be broken since they obviously have ancient compat dates as well. (In fact... they predate compat dates...)

These were intended to be convenient wrappers around `fetch()` that perform the respective HTTP method. It was a bad idea (my fault). Luckily it was never documented and probably few people depend on it.

Note that a long time ago, KV namespace bindings were Fetchers rather than being the specific KvNamespace type. This had lots of problems, but sort of worked as long as your key names were URL-safe. It's possible very old scripts are in production that still depend on these old-style bindings, but they won't be broken since they obviously have ancient compat dates as well. (In fact... they predate compat dates...)
@kentonv kentonv requested a review from jasnell March 5, 2024 23:09
@kentonv kentonv requested review from a team as code owners March 5, 2024 23:09
@kentonv kentonv requested a review from fhanau March 5, 2024 23:09
@@ -12,13 +12,21 @@ export class DurableObjectExample {
foo() {
return 123;
}

fetch(req) {
return new Response(req.method + " " + req.url);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return new Response(req.method + " " + req.url);
return new Response(`${req.method} ${req.url}`);

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed that's an improvement. Don't think it's worth restarting CI, though.

@irvinebroque
Copy link
Collaborator

This is actually a pretty cool idea and TIL it existed! Very similar to Pages Functions.

kentonv added a commit to cloudflare/cloudflare-docs that referenced this pull request Mar 5, 2024
@kentonv
Copy link
Member Author

kentonv commented Mar 6, 2024

The "linux" build seems to have hung on mksnapshot, but "linux-debug" passed and all internal builds passed, and GitHub apparently cannot cancel the hung workflow, it's just gonna run until it times out or something. I'm just going to merge.

@kentonv kentonv merged commit 37a0b39 into main Mar 6, 2024
9 of 11 checks passed
@kentonv kentonv deleted the kenton/fetcher-no-get-put-delete branch March 6, 2024 00:04
kentonv added a commit to cloudflare/cloudflare-docs that referenced this pull request Mar 6, 2024
* Document compat flag: fetcher_no_get_put_delete

As implemented in: cloudflare/workerd#1770

* Update content/workers/_partials/_platform-compatibility-dates/fetcher-no-get-put-delete.md

* Apply suggestions from code review

Co-authored-by: Brendan Irvine-Broque <[email protected]>

---------

Co-authored-by: Brendan Irvine-Broque <[email protected]>
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.

3 participants