WICG / pending-beacon

A better beaconing API
Other
43 stars 8 forks source link

consider adapting window.fetch API for beaconing #52

Closed fergald closed 9 months ago

fergald commented 1 year ago

@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

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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.

annevk commented 1 year ago

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 commented 1 year ago

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 commented 9 months ago

The API is repurposed into fetchLater() https://github.com/whatwg/fetch/pull/1647 after https://github.com/WICG/pending-beacon/issues/70.