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

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

Closed
mingyc opened this issue Mar 24, 2023 · 17 comments
Closed

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

mingyc opened this issue Mar 24, 2023 · 17 comments
Labels
api Issue with API specs discussion fetch-based-api Fetch-based API design

Comments

@mingyc
Copy link
Collaborator

mingyc commented Mar 24, 2023

Context: #70 and #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:

  • keepalive: must be true. {deferSend: new DeferSend(), keepalive: false} conflicts with each other.
  • url: supported.
  • method: one of GET or POST.
  • headers: supported.
  • body: only supported for POST.
  • signal: supported.
  • credentials: enforcing same-origin to be consistent.
  • cache: not supported?
  • redirect: enforcing follow?
  • referrer: enforcing same-origin URL?
  • referrerPolicy: enforcing same-origin?
  • integrity: not supported?
  • priority: enforcing auto?

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 mingyc added discussion api Issue with API specs labels Mar 24, 2023
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 Author

mingyc commented Mar 28, 2023

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

@mingyc mingyc added the fetch-based-api Fetch-based API design label Mar 28, 2023
@yoavweiss
Copy link
Collaborator

^^ @noamr

@noamr
Copy link
Contributor

noamr commented Mar 28, 2023

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
Copy link
Collaborator Author

mingyc commented Mar 29, 2023

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

@noamr
Copy link
Contributor

noamr commented Mar 29, 2023

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
Copy link

annevk commented Mar 29, 2023

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
Copy link
Collaborator Author

mingyc commented Mar 30, 2023

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
Copy link

annevk commented Mar 30, 2023

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
Copy link
Collaborator

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
Copy link

annevk commented Mar 30, 2023

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

@yoavweiss
Copy link
Collaborator

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
Copy link

annevk commented Mar 30, 2023

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
Copy link
Collaborator Author

mingyc commented Mar 31, 2023

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
Copy link

annevk commented Mar 31, 2023

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
Copy link
Collaborator Author

mingyc commented Apr 4, 2023

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

@annevk
Copy link

annevk commented Apr 4, 2023

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

mingyc added a commit to mingyc/pending-beacon that referenced this issue Jul 4, 2023
mingyc added a commit to mingyc/pending-beacon that referenced this issue Jul 4, 2023
mingyc added a commit to mingyc/pending-beacon that referenced this issue Jul 4, 2023
mingyc added a commit to mingyc/pending-beacon that referenced this issue Jul 4, 2023
mingyc added a commit that referenced this issue Jul 4, 2023
This PR adds overview and example codes for the draft `fetchLater()` API spec from whatwg/fetch#1647

The API will address the discussions in #70 #72 #73 #74 #75 #76.
@mingyc
Copy link
Collaborator Author

mingyc commented Nov 13, 2023

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

  • keepalive flag is enforced to true.
  • A request limit is enforced across the requests from the same domain.
  • Streaming requests are not allowed.
  • Others, see the details in the spec.

@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 fetch-based-api Fetch-based API design
Projects
None yet
Development

No branches or pull requests

4 participants