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

fetchLater() API (previously: PendingBeacon API) #85

Closed
mingyc opened this issue Nov 9, 2022 · 20 comments
Closed

fetchLater() API (previously: PendingBeacon API) #85

mingyc opened this issue Nov 9, 2022 · 20 comments
Assignees
Labels
concerns: API design The API for this is error-prone, poorly named, or inconsistent with the platform concerns: complexity This proposal seems needlessly complex concerns: integration Can't be used w/ other web platform features (or unclear what happens if used together) concerns: maintenance It's not clear the proposal is getting maintained. concerns: privacy This proposal may cause privacy risk if implemented concerns: venue This proposal is in the wrong standards/incubation venue, or it's not in a venue at all from: Google Proposed, edited, or co-edited by Google. position: support topic: loading topic: web apis Spec relates to web APIs (entry points for script) venue: WICG Proposal is incubated in the Web Incubator Community Group

Comments

@mingyc
Copy link

mingyc commented Nov 9, 2022

Request for position on an emerging web specification

  • WebKittens who can provide input:

Information about the spec

Design reviews and vendor positions

Anything else we need to know

We would like to have feedback from WebKit about this API before finalizing its shape and behavior.

@gsnedders gsnedders added topic: loading topic: web apis Spec relates to web APIs (entry points for script) venue: WICG Proposal is incubated in the Web Incubator Community Group from: Google Proposed, edited, or co-edited by Google. labels Nov 10, 2022
@othermaciej
Copy link

How does this relate to Beacon API? Why is it a separate spec and not an extension of Beacon API?

Also on first glance, this is a lot more complicated than Beacon API itself, which is just one method, not a class hierarchy of multiple interfaces. Is the extra complexity actually necessary? At first glance, it's not obvious why it is necessary to have all these ways to customise a "pending" beacon when regular beacons don't need these things. (If Beacon API itself is underpowered, then that would be more reason to extend the Beacon API instead of making an orthogonal API).

@othermaciej
Copy link

Also needs to be examined to see if it enables cross-site tracking.

@othermaciej othermaciej added concerns: complexity This proposal seems needlessly complex concerns: privacy This proposal may cause privacy risk if implemented concerns: integration Can't be used w/ other web platform features (or unclear what happens if used together) labels Dec 20, 2022
@mingyc
Copy link
Author

mingyc commented Dec 21, 2022

I'll try to answer some of your questions. Feel free to add more under https://github.com/WICG/pending-beacon/issues so that more pepole can discussion

How does this relate to Beacon API? Why is it a separate spec and not an extension of Beacon API?

At the beginning there were discussions around the following options

  1. extending navigator.sendBeacon() with a flag: the use cases went beyond a single flag. They need to be able to update what will be sent and/or cancel it. But the existing sendBeacon() semantic doesn't support it.
  2. adding a new declarative DOM API: but scripting will be neccessary to fill data.
  3. [chosen] adding a new stateful JS API.

1 and 2 have its own drawbacks, and the 3rd one is chosen. @fergald might be able to provide full context.

Also on first glance, this is a lot more complicated than Beacon API itself, which is just one method, not a class hierarchy of multiple interfaces. Is the extra complexity actually necessary?

It's answered in WICG/pending-beacon#59. In short we would like to provide more type safety on compile time.

At first glance, it's not obvious why it is necessary to have all these ways to customise a "pending" beacon when regular beacons don't need these things. (If Beacon API itself is underpowered, then that would be more reason to extend the Beacon API instead of making an orthogonal API).

@mingyc
Copy link
Author

mingyc commented Dec 21, 2022

Also needs to be examined to see if it enables cross-site tracking.

The scope of this API proposal doesn't include cross-site tracking.

Also cc @fergald

@mingyc
Copy link
Author

mingyc commented Jan 19, 2023

@othermaciej pinging for any updates or comments

@mingyc
Copy link
Author

mingyc commented Feb 2, 2023

Friendly pinging again for your position.

How does this relate to Beacon API? Why is it a separate spec and not an extension of Beacon API?

In addition to the above reasons from #85 (comment), we further added a section to explain why not extending the Fetch API (Beacon API is roughly equivalent to Fetch keepalive for Post-only request). Simply put, our requirements are beyond the scope of Fetch.

@marcoscaceres
Copy link
Contributor

marcoscaceres commented Feb 6, 2023

I’m also concerned about adding an entirely new API.

They need to be able to update what will be sent and/or cancel it. But the existing sendBeacon() semantic doesn't support it.

sure, but I’m wondering if all possible options have been explored for extending the existing spec? For instance, perhaps a third argument could be added to sendBeacon() accepting a dictionary that:

  • takes an optional AbortController’s signal
  • takes an optional http method name, so to support POST.
  • takes the timeout.

Perhaps something like the above could addressed the use cases without needing an entirely new API?

@marcoscaceres marcoscaceres added the concerns: venue This proposal is in the wrong standards/incubation venue, or it's not in a venue at all label Feb 6, 2023
@marcoscaceres
Copy link
Contributor

Just also want to clarify that the feedback isn’t specifically about the API shape, but rather the maintenance of the existing specification and infrastructure. Forking a new spec would be quite undesirable because the existing beacon spec is already not well maintained (I.e., now we have two problems). Echoing @othermaciej, we’d rather see the standards community work on updating what already exists specs than introduce something new that mostly replicates and extends existing functionality. It could also help address issues raised by Mozilla folks too.

Adding “venue” as a concern, as doing this work in WebPerfWG may be preferable.

@fergald
Copy link

fergald commented Feb 6, 2023

@marcoscaceres Thanks for the response.

We'll update the explainer to explore modifying sendBeacon however I don't think adding an AbortController can meet the requirements. As well as being able to abort, being able to tell whether the payload has been sent already is a key feature, without that it's impossible to accumulate data and avoid resending.

That requires something that's accessible and represents the beacon pending state. We could provide a way to supply a callback to be called if the beacon is sent and the caller could then track that state but ergonomically that seems poor.

An alternative would be to have sendBeacon return an object instead of true when used with the new features. That would make sendBeacon a factory for PendingBeacon objects. So we still end up with Beacon objects with mostly the same features. Perhaps that's a little more appealing.

@yoavweiss
Copy link

In terms of spec mechanics and venue, the WebPerfWG have been talking about potentially adopting this API and merging it into the beacon spec. If we have y'all's support for this, we can easily do that.

In terms of API shape, @fergald led a few interesting discussions (at TPAC, as well as before and after) about the API at the WG, which led to the current proposal. As Fergal said, there are significant differences between sendBeacon's "send this data immediately, but have it survive a renderer shutting down" semantics, vs. PendingBeacon's "send this data once the renderer shuts down" semantics. Those differences lead to 2 different API shapes.

IMO, trying to cram everything into a single API would only lead to confusion (and I'm not even sure what that would look like). At the same time, if y'all want to have a high bandwidth discussion about that at the WebPerfWG, I'm more than happy to facilitate that.

^^ @achristensen07

@annevk annevk added the concerns: API design The API for this is error-prone, poorly named, or inconsistent with the platform label Feb 8, 2023
@annevk
Copy link
Contributor

annevk commented Feb 8, 2023

The WebPerf WG has historically done a poor job of Fetch integration. Getting that done correctly is somewhat vital for this API. The note in the draft is not reassuring. I filed WICG/pending-beacon#63 on that.

Some of @domenic's ideas on sharing more API surface in WICG/pending-beacon#52 (comment) make sense to me, but if maintenance is not centralized that still seems like a danger zone (see Cache API). (The API also seems like too much a departure from fetch() for something that is so similar. Note that sendBeacon() had that problem as well and we essentially eventually folded it into fetch() therefore.)

@marcoscaceres
Copy link
Contributor

marcoscaceres commented Feb 9, 2023

Those differences lead to 2 different API shapes.
IMO, trying to cram everything into a single API would only lead to confusion (and I'm not even sure what that would look like).

I guess it depends on what the goals are. If one of the goals would be to keep this new API backwards compatible, then one could add the optional third argument with the fetchOptions dictionary @domenic suggested and perhaps change the return signature to a (boolean or unsigned long). As with Geolocation's watchPosition() and HTML's setInterval(), the API can then return a handle which could be used to look up the state of a beacon. So .sendBeacon(url, data, fetchOptions), returns:

  • true/false - legacy behavior.
  • > 1 - id to look up state (e.g., 3).

Then one can add something like .checkBeaconState(3);, which could return an enum value of "pending", "aborted" (possibly via signal), "sent" or whatever.

Anyway, point being, it's good having a set of agreed upon goals requirements and hopefully getting general agreement that (API shape aside!) "Pending Beacons" as a general concept are a desirable thing we want to collectively add the the Web Platform. At the same time, the concerns that @annevk and I have raised about integration, maintenance, and venue need a concrete plan that we (WebKit) can be confident in.

I think we will get there, but just need to work out the details. Give us a few days to discuss the general idea amongst ourselves and we will get back to you soon with a position (and hopefully a path forward to collaborative on making something great).

@marcoscaceres marcoscaceres added the concerns: maintenance It's not clear the proposal is getting maintained. label Feb 13, 2023
@marcoscaceres
Copy link
Contributor

marcoscaceres commented Feb 13, 2023

After consideration and discussion with colleagues, we express our tentative support for the standardization effort with some concerns, as indicated by the GitHub labels. To collaboratively address the concerns:

  • We would like to see further refinement of the API design. While we appreciate the proposed API in the Explainer, we believe there is room for improvement and suggest we explore alternative solutions.
  • The integration of Fetch, as raised by @annevk, is crucial for security reasons and must be addressed.
  • For maintenance and integration reasons, we are concerned about the standardization effort being carried out in the WICG. While the scope of this proposal is broader, we would prefer to see it either integrated into the existing Beacon API specification or into the Fetch Standard as a new chapter.
  • The specification should include a size limit on the data to be transmitted (it's eluded to already, but something we should concretely work out).
  • The specification should also impose a reasonable limit on the number of pending beacons, such as to 1 or 2.

Our colleagues and others in the WebKit community may have additional points to raise. If not, we will consider this position to be "supportive" within the next week or so.

@mingyc
Copy link
Author

mingyc commented Feb 16, 2023

While the scope of this proposal is broader, we would prefer to see it either integrated into the existing Beacon API specification or into the Fetch Standard as a new chapter.

@marcoscaceres Could you help clarify the integration here?
Is it about the existing JS navigator.sendBeacon() or fetch() is preferred over new API, or about the underlying algorithms need to be the same as/integarted into Fetching? If it's the latter, do you have any further comments about the current proposed API shape?

Thanks!

@marcoscaceres
Copy link
Contributor

marcoscaceres commented Feb 20, 2023

Is it about the existing JS navigator.sendBeacon() or fetch() is preferred over new API,

Not necessarily. It's more that we have those two specs (Beacon and Fetch) already and, irrespective of API shape and new functionality, this new proposal has to integrate with both of those APIs: the existing spec infrastructure. We know there is new functionality being added, but it's still going to have to go into the algorithms of fetch in particular, and hopefully into the existing algorithms of the Beacon spec. If it doesn't, then that would be somewhat weird given it's a "beacon" even though it's "pending".

Further, if we end up with a new spec, it means that Pending Beacon might not benefit from enhancements made to either Fetch or the existing Beacon spec. That's why we want it integrated into either of the existing specs.

or about the underlying algorithms need to be the same as/integarted into Fetching?

That's correct (even if it's a super special fetch that survives a browser crash or whatever).

If it's the latter, do you have any further comments about the current proposed API shape?

I don't have any further comments about the API shape at this point - but we've outlined some of the problems already. For instance, this isn't an API where developers will create unlimited instances of an object: it's going to be heavily restricted to a couple of requests, maybe even just 1, and probably at most 2? (i.e., a new PendingBeacon() shape might not make a lot of sense because it encourages creating a lot of objects).

Further we have an existing API shape (navigator.sendBeacon()), and it doesn't feel like we've fully explored retrofitting it to do the new thing we want pending beacon to do.

mingyc added a commit to mingyc/pending-beacon that referenced this issue Mar 8, 2023
This summarize the discussion from
WebKit/standards-positions#85 (comment)
and also potentially unblocks the need to add custom headers in WICG#50
mingyc added a commit to WICG/pending-beacon that referenced this issue Mar 9, 2023
@hober hober moved this to Provisional position found in Standards Positions Review Backlog Mar 23, 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#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 to WICG/pending-beacon 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
@hober hober closed this as completed Mar 27, 2023
@github-project-automation github-project-automation bot moved this from Position identified to Done in Standards Positions Review Backlog Mar 27, 2023
@marcoscaceres
Copy link
Contributor

An update proposal is shown in this video. We should probably engage in the appropriate forum.

@mingyc
Copy link
Author

mingyc commented Mar 29, 2023

There is an ongoing discussion about fetch based API proposal. Please take a look at the main thread WICG/pending-beacon#70 and this list of discussions https://github.com/WICG/pending-beacon/issues?q=is%3Aissue+is%3Aopen+label%3Afetch-based-api

@hober hober added blocked Coming to a position is blocked on issues identified with the spec or proposal. and removed position: support labels Mar 29, 2023
@hober hober reopened this Mar 29, 2023
@hober hober moved this from Unscreened to Blocked in Standards Positions Review Backlog Mar 29, 2023
@annevk
Copy link
Contributor

annevk commented Apr 27, 2023

We put forward a proposal that would remove a number of our concerns here and would make us supportive of this effort: WICG/pending-beacon#70 (comment).

@marcoscaceres marcoscaceres added position: support and removed blocked Coming to a position is blocked on issues identified with the spec or proposal. labels May 5, 2023
@marcoscaceres
Copy link
Contributor

Marking as support given the progress made...

@mingyc
Copy link
Author

mingyc commented Jul 18, 2023

Note: the PendingBeacon API has been evolved to a fetch-based approach, and the latest discussions happen in whatwg/fetch#1647

@mingyc mingyc changed the title PendingBeacon API fetchLater() API (previously: PendingBeacon API) Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concerns: API design The API for this is error-prone, poorly named, or inconsistent with the platform concerns: complexity This proposal seems needlessly complex concerns: integration Can't be used w/ other web platform features (or unclear what happens if used together) concerns: maintenance It's not clear the proposal is getting maintained. concerns: privacy This proposal may cause privacy risk if implemented concerns: venue This proposal is in the wrong standards/incubation venue, or it's not in a venue at all from: Google Proposed, edited, or co-edited by Google. position: support topic: loading topic: web apis Spec relates to web APIs (entry points for script) venue: WICG Proposal is incubated in the Web Incubator Community Group
Development

No branches or pull requests

9 participants