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

consider adapting window.fetch API for beaconing #52

Closed
fergald opened this issue Oct 20, 2022 · 7 comments
Closed

consider adapting window.fetch API for beaconing #52

fergald opened this issue Oct 20, 2022 · 7 comments
Labels
api Issue with API specs discussion

Comments

@fergald
Copy link
Collaborator

fergald commented Oct 20, 2022

@annevk @philipwalton @clelland

Forking off from this comment.

It's not that keepalive is fundamentally broken, it's that the there is reliable way to initiate these kinds of fetches from JS and there cannot be. You must either oversend or accept losing a lot of fetches (especially on mobile).

If we really wanted to keep using window.fetch, then we could perhaps add a defer-send option.

Such a fetch would

  • not send immediately (and would behave like a beacon for its send timing)
  • maybe never resolve its promise, certainly the caller should not expect it to happen but it might be OK to resolve those that are sent before the page is destroyed. It might be better to consistently never resolve, so that nobody depends on it.

The existing abort signal option would give us control over cancelling. We would probably want more options that only apply to deferred fetches to deal with timing.

Are there options on fetches that we would want to disallow for a deferred fetch?

@fergald
Copy link
Collaborator Author

fergald commented Oct 20, 2022

Alternatively, to avoid the piecemeal growth of the BeaconAPI to support more of fetch's options, we could allow all of fetch-options to be passed to a beacon. There are some that might not make sense but we should exclude rather than gradually include.

@ricea
Copy link

ricea commented Oct 21, 2022

The idea of a promise that never resolves makes me uncomfortable. An alternative would be to resolve immediately with some kind of "empty" Response, though I don't concretely know what that would look like.

I favour refusing to fetch and returning a rejection if defer-send is used together with any unsupported options. This would permit supporting a larger section of fetch options in future without breaking existing pages.

Adding a timeout feature that only worked for defer-send would be odd, so we'd have to make it work for all kinds of fetch, which would be weird after rejecting the idea for so long.

@fergald
Copy link
Collaborator Author

fergald commented Oct 21, 2022

@ricea

We would probably want defer-send to take a set of options actually since timeout for defer-send is related to how long before sending but with any other fetch it would mean how long to try sending (beacon might also want that). Also, I can other options for controlling resends in the future.

Also given my other comment it might not be a good idea to have such a difference in behaviour in terms of header processing just based on an option.

TBH, it doesn't seem like a great fit. I'm leaning towards Beacon should take a fetch-options input rather than fetch should be able to act like beacon.

@domenic
Copy link

domenic commented Oct 27, 2022

I think reusing fetch() itself would be a mistake. This API is fundamentally different from, and greater than, fetch()-with-keepalive. You can think of it as really a combination of two low-level things:

  1. A super-reliable mechanism for delaying work until page unload, including e.g. after bfcache eviction or browser crashes;
  2. Doing a keepalive fetch when that mechanism triggers.

Most of the interesting parts of this API are in (1). And we don't want to decouple them, because (1) by itself is too much power to give developers. We want them running less JavaScript at unload-event time, not more, and we don't want them running any code at all during bfcache eviction or after browser crashes.

I think turning fetch() from an API that does (2), into an API that also bundles in (1), would be a mistake. (1) is too disconnected from the fetching process, and the API that makes sense for fetch() makes much less sense for (1). For example, the idea of returning a promise, or the entire Response class, don't make sense for this unload beacon API. And all the timeout and pagehide-related stuff don't make sense for the fetch() API.

A strained analogy would be how image loading is a no-cors fetch plus drawing the image. We don't extend no-cors fetch() to also have image drawing capabilities; we instead build a separate image-loading-and-displaying API (the <img> element).


All that said, I think we could consider ways to align the surface area of the unload beacon API, especially its inputs, with fetch() and its inputs. E.g., maybe instead of new PendingPostBeacon(url, { timeout, backgroundTimeout }) with a setData() method, you should have new PendingBeacon(url, { timeout, backgroundTimeout, fetchOptions }) where fetchOptions contains some subset of RequestInit. (Off the top of my head, method, headers, some values for body, referrer, referrerPolicy, credentials, and cache all seem reasonable to add.) Or some variant of that, e.g. mixing in the fetch options instead of nesting them, or passing them as a third parameter.

Continuing the strained analogy from the above, this would be the equivalent of replacing <img>'s various ad-hoc fetch customization options (referrerpolicy="", crossorigin="") with a unified fetchoptions="" mechanism. Which is something we've kind of wanted to do for a while, except that putting JSON into HTML attributes is icky, and there hasn't been any super-compelling need, and we'd have to figure out how the two customization vectors interact.

@mingyc mingyc added discussion api Issue with API specs labels Oct 28, 2022
mingyc added a commit to mingyc/pending-beacon that referenced this issue Feb 2, 2023
This section summarize the discussions from WICG#52 and WICG#50
mingyc added a commit that referenced this issue Feb 2, 2023
This section summarize the discussions from #52 and #50, which suggests that Fetch API doesn't fit the requirements for beaconing at page discard.
@annevk
Copy link

annevk commented Feb 8, 2023

Part of my worry here is around API surface and keeping everything synchronized. This was problematic with sendBeacon(). To this day it is problematic with the Cache API. This will be another source of mismatches, especially if this API is not maintained as part of the Fetch Standard.

@annevk
Copy link

annevk commented Feb 8, 2023

Though having looked a bit more at the classes this does seem like a wild departure from fetch() if the main differences are that there's no return value and you can update what gets transmitted.

And it also has weird restrictions such that you can update the body with POST, but not the URL.

mingyc added a commit to mingyc/pending-beacon that referenced this issue Mar 24, 2023
- Previous discussions about fetch-based API are in WICG#70, WICG#52, WICG#50, WebKit/standards-positions#85 (comment)
- Related new discussions in WICG#71, WICG#72, WICG#73, WICG#74, WICG#75
mingyc added a commit to mingyc/pending-beacon that referenced this issue Mar 24, 2023
- Previous discussions about fetch-based API are in WICG#70, WICG#52, WICG#50, WebKit/standards-positions#85 (comment)
- Related new discussions in WICG#71, WICG#72, WICG#73, WICG#74, WICG#75
mingyc added a commit to mingyc/pending-beacon that referenced this issue Mar 24, 2023
- Previous discussions about fetch-based API are in WICG#70, WICG#52, WICG#50, WebKit/standards-positions#85 (comment)
- Related new discussions in WICG#71, WICG#72, WICG#73, WICG#74, WICG#75
mingyc added a commit to mingyc/pending-beacon that referenced this issue Mar 24, 2023
- Previous discussions about fetch-based API are in WICG#70, WICG#52, WICG#50, WebKit/standards-positions#85 (comment)
- Related new discussions in WICG#72, WICG#73, WICG#74, WICG#75, WICG#76, WICG#77
mingyc added a commit that referenced this issue Mar 24, 2023
- Previous discussions about fetch-based API are in #70, #52, #50, WebKit/standards-positions#85 (comment)
- Related new discussions in #72, #73, #74, #75, #76, #77
@mingyc
Copy link
Collaborator

mingyc commented Nov 13, 2023

The API is repurposed into fetchLater() whatwg/fetch#1647 after #70.

@mingyc mingyc closed this as completed Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issue with API specs discussion
Projects
None yet
Development

No branches or pull requests

5 participants