WICG / pending-beacon

A better beaconing API
Other
43 stars 8 forks source link

[Fetch-Based API] Limiting the scope of pending requests #72

Closed mingyc closed 9 months ago

mingyc commented 1 year ago

Context: #70 and https://github.com/WICG/pending-beacon/pull/71

Even if moving toward a fetch-based design, this proposal does still not focus on supporting every type of requests as beacons.

For example, it's non-goal to support most of HTTP methods, i.e. being able to defer an OPTION or TRACE until page destroyed. It aldo does not make sense to allow setting keepalive: false for a deferred request.

We should look into fields of RequestInit and decide whether deferSend should throw errors on some of their values:

As shown above, at least keepalive: true and method need to be enforced.

If going with this route, can we also consider the PendingRequest API approach that proposes a subclass of Request to enforce the above?

mingyc commented 1 year ago

@yoavweiss @clelland @annevk Could you please take a look?

yoavweiss commented 1 year ago

^^ @noamr

noamr commented 1 year ago

See this comment. I think that it's OK if we define that deferred fetches have to be keepalive. The rest are perhaps not that necessary? We can also mandate GET/POST but I'm not sure that's necessary either.

mingyc commented 1 year ago

Do do you prefer to limit the request? Just throwing TypeError?

noamr commented 1 year ago

Do do you prefer to limit the request? Just throwing TypeError?

Yes like keepalives. Remove the responsibility of having to auto flush partial beacons while the doc is alive from fetch. Each origin manages its own partial beacon and if the quota is exceeded it's a network error.

This would meet the requirement of "the minimal additions to fetch to make the use case possible"

annevk commented 1 year ago

Hey @mingyc, sorry for not having been part of the earlier discussions, but when you say "non-goal" it would really help if that came with a link. It's not clear to me why we would not allow some or all of these things to be as expressive as fetch() is.

mingyc commented 1 year ago

It comes from the motivation for this proposal, that we want to provide an alternative to calling addEventListener('unload', ()=> navigator.sendBeacon()); for analytic purpose, which actually only support POST with data. Even going with fetch()-based approach, we don't think it's a requirement to support all possible types of requests.

annevk commented 1 year ago

At some point navigator.sendBeacon() was effectively replaced by fetch(..., { keepalive: true }) because it was insufficiently capable. Are you saying that's no longer the case somehow?

Aside from it seeming harder to target a subset of the API, I'm also worried that a couple years from now we'd be in the same boat as we were with navigator.sendBeacon() if we go down that route.

yoavweiss commented 1 year ago

I think it may make sense to currently limit implemented support to GET and POST, while still enabling future extension for other methods, if that becomes required. For that, we'd need to think of a way for developers to recognize lack of support in the future (e.g. by rejecting the promise with a NotSupportedError DOMException for not-yet-supported methods).

annevk commented 1 year ago

I guess I don't really understand why we would limit them?

yoavweiss commented 1 year ago

I guess I don't really understand why we would limit them?

My guess is that it can reduce initial implementation complexity, and is not something that's currently required by the potential users of the API.

annevk commented 1 year ago

I think for WebKit it would likely end up being more complex. And if it comes with feature detection requirements there are also API complexity costs.

mingyc commented 1 year ago

At some point navigator.sendBeacon() was effectively replaced by fetch(..., { keepalive: true }) because it was insufficiently capable. Are you saying that's no longer the case somehow?

I don't know about the details when the API was released. But just looking at our recent statistics, navigator.sendBeacon() itself has 20% more daily usage then the entire fetch() API, and fetch(...,{keepalive:true}) itself is one order of magnitude smaller. I wouldn't say that navigator.sendBeacon() is effectively replaced.

Given that, I think we can justify supporting a general request is not our goal?

annevk commented 1 year ago

That's a surprising statistic. Would love to see HTTPArchive crawl data as to why that is the case.

To the topic at hand, navigator.sendBeacon() is a thin wrapper around fetch and fetch is strictly more capable. I'm still not sure why we'd introduce another networking API that is less capable.

mingyc commented 1 year ago

Could you suggest how we should proceed with this (fetch-based) proposal?

annevk commented 1 year ago

@noamr has been working on that in #70, no?

mingyc commented 9 months ago

fetchLater() API spec nows take the same RequestInfo object as fetch(), and a DeferredRequestInit which extendes RequestInit, with some internal differences like