Add method to sendBeacon

[toddreifsteck]

One blocker for adoption of sendBeacon has been that GET is missing which many analytics services depend on today. This PR removes that restriction by adding the ability to specify the method on the request.

This PR should satisfy #22

[igrigorik]

@toddreifsteck overall, this looks reasonable. Couple of quick questions:

• should we allow methods other than GET? As written, OPTIONS, HEAD, PUT, DELETE, are all allowed and I'm thinking we probably don't want to enable that? E.g. DELETE via beacon seems weird and dangerous.
• if I pass sendBeacon({url: ...}, body), I think that will fail with TypeError. Should we default to POST if value is omitted?

/cc @annevk @ehsan ptal.

[annevk]

Removing the Oxford comma. Bold move.

[toddreifsteck]

With regard to other methods than GET, I informally reviewed adding GET with a few folks at Microsoft. 2 said "Why are you limiting to GET?" and thus I removed the restriction to prompt this conversation. It is a BIT odd to limit to only GET/POST rather than just passing the method through. Also.. HEAD is arguably a better choice than GET for all GET scenarios and I could argue that it should be allowed. I'm 100% open to feedback.

With regard to missing method, I'm happy to default or to throw. No opinion.

[annevk]

Given you get a CORS preflight if the beacon goes across origins it doesn't seem like a concern that you allow any method. Assuming it invokes Fetch in the right way which I guess it does. I'm a little concerned we now have three APIs that need to do the proper thing around method checking and normalization, as well as forbidding a body when the method is GET/HEAD, but we might be able to manage.

[igrigorik]

@toddreifsteck fair enough. Although, I'm scratching my head over OPTIONS + sendBeacon. :)

[toddreifsteck]

Per call on 2/3, I'll update PR to default to POST.

[toddreifsteck]

Had a pre-TAG review with @travisleithead and per that review, I've got another update in process to move the object to the 2nd parameter rather than the first. Will discuss on the 2/17 call then ETA is 2/17 for updated PR based upon results of conversation.

[toddreifsteck]

Beacon issues to address--

1. First parameter is where overload needs to be
2. Should first parameter be a dictionary or an object?
3. Send this over to TAG when PR is updated
4. Ensure that Ilya's feedback from earlier is addressed.

[toddreifsteck]

With regard to issue 2 from above:
Spoke with @DigiTec about dictionary vs. object. He made a good argument for using object for the first parameter. If there are current users that use an object with a toString overload such as https://developer.mozilla.org/en-US/docs/Web/API/Location for this parameter, adding a dictionary overload will break them. Because of this, the first parameter will be USVString or BeaconRequest as currently specced.

[toddreifsteck]

@travisleithead , please take a look!

[toddreifsteck]

A lot of discussion has occurred internally on why we've chosen a type on parameter 1.

There are 6 options for solving the addition of method that I’m aware of:

1. Choosing to break .ToString URL overloads on the 1st param
2. Choosing NOT to allow BodyInit to ever be .ToJson on an unknown object by default on the 2nd param
3. Add a 3rd param. (For a GET, this requires a null 2nd param)
4. CHOSEN--Add a full Type to 1st param (big dev downside.. but the only true back-compatible choice without adding new API)
5. Add a full Type to 2nd param. (Same downside as 2. Makes it harder to add .ToJson on BodyInit if we overload that param.)
6. BEST but.. CRAZIEST--Don’t change this method AT ALL. Instead, make a Beacon type. It is ctored with a dictionary. It has a “correct” overload. We deprecate navigator.sendBeacon.

[travisleithead]

6 is interesting. If back-compat is such a large concern that you are weighing the developer ergonomics against it, I think you need to seriously consider bikeshedding a new [but perhaps related] name that doesn't have a pre-existing implementation burden. What would the API look like ideally (without worrying about existing back-compat or interop)?

[toddreifsteck]

The entire purpose of this PR is to extend an existing API to overcome the main reason why adopters choose not to use it. I'm unsure whether it is worth the cognitive cost of adding a new API to achieve this goal for a better dev experience. However... I'll play along.

If Navigator.sendBeacon did not already exist and my purpose were NOT to extend an existing API, I believe I would naively create a new type, Beacon, exposed in all contexts. I would initialize it with a dictionary containing url/method. I would have 1 method, send. That method would take a dictionary containing url, method and a nullable BodyInit. This API would be easier to extend if we found new ways to improve it. It could also be constructed once and re-used which could lead to simpler code in some scenarios.

I'd also want to touch base with @annevk and @igrigorik and get their take as it is possible I'm missing something.

[travisleithead]

4 is definitely ugly; the BeaconRequest new type must be instantiated by the implementation, only to be immediately thrown out after the API call in most cases :( I suppose my preference is the break (1) by just using the dictionary.

[annevk]

Oh, I hadn't realized BeaconRequest was a class. Why is that not a dictionary?

[toddreifsteck]

@annevk The reason it is not being added as a dictionary is to avoid potentially breaking code that depends on an object's .toString() that returns a url for the first parameter in Navigator.sendBeacon(url, body).

I am happy to update it to be a dictionary if @annevk and @igrigorik give me the go ahead.

(It is not clear there is a good way to know if this occurs today without one of the UAs adding telemetry to know for sure.)

[DigiTec]

A real use case might be something that stringifies to url automatically. Like a location object.

[annevk]

I see, or a URL object. I guess that's a reasonable concern.

An alternative here is that we overload fetch() with something although I'm not quite sure what we should do about the return value. Another alternative would be to introduce fetchBeacon() with a better signature. Not sure I like any of these.

[toddreifsteck]

fetchBeacon would be very simple but.. sloppy as we have to add a new API to have a super clean dev-friendly interface.

Based upon all data below, it is my belief that there is no "good" choice to upgrade sendBeacon or to upgrade fetch.

Given that, I believe options 1 or 2 below are the choices we must choose between. (1 seems more similar to existing network APIs and would be my preferred choice for consistency but @DigiTec had strong arguments for option 2.)

Restating Options:
1- Navigator.fetchBeacon(url, { method, body }) (perhaps on a different namespace)
Drawback would be that it is a new API.
Pro is that it has no back-compat issues.

2- new Beacon type with a .send(url, { method, body }) method.
Drawback would be that it is a new API and is a unique networking API as it has an object.
Pro is that it has no back-compat issues.

3- Update Navigator.sendBeacon(url, body) to Navigator.sendBeacon(url or BeaconRequestType, body)
Drawback is that it is painful to use.
Pro is that it has no back-compat issues.

4- Update Navigator.sendBeacon(url, body) to Navigator.sendBeacon(url or BeaconRequestDictionary, body)
Drawback is that it blocks .ToString types such as URL from being passed raw to first param. (I assert that this problem 100% disqualifies this overload and undoes the friendliness.)
Pro is that it is very friendly to use.

5- Update Navigator.sendBeacon(url, body) to Navigator.sendBeacon(url, BodyInit or NewThing)
Drawback is that it prevents BodyInit from being updated to default to .toString or JSON.stringify() in the future. (Per @igrigorik , there is discussion on that occurring.)
Pro is that url works fine.

[igrigorik]

What about the earlier option of third parameter? Technically, GET is allowed to have a body - just sayin.

partial interface Navigator {
    boolean sendBeacon(USVString url, optional BodyInit? data = null, optional BeaconOptions? opts);
};

dictionary BeaconOptions {
    string method = "POST";
};

Or some such?

[toddreifsteck]

Although HTTP allows for a GET with a body, the Request object in the Fetch spec currently does not. See line 31 of the processing algorithm for Request Class

XHR ignores the body when 'GET' or 'HEAD' is set per Send method spec

We could choose one of those behaviors and implement a 3rd parameter... which would work but.. is also kind of ugly....

[igrigorik]

@toddreifsteck yep, that was more tongue in cheek on my part :) That said, any reason why you omitted the third argument option in your earlier summary?

[toddreifsteck]

Accidental :) In my head, I keep axing it as ugly but it may be a valid option... I'm going to circle with @travisleithead and @DigiTec tomorrow and will share our thinking by EOD.

[annevk]

I think my current favorite is fetchBeacon() specified alongside fetch() (on the same object) with navigator.sendBeacon() as its legacy lesser alternative. fetchBeacon() basically matches fetch() except it doesn't have a return value and the user agent has some flexibility when it actually does the fetch.

[plehegar]

I agree that, in design, it matches fetch() otherwise but it would be confusing imho to call the method fetchBeacon when it isn't meant to fetch things. emitBeacon is an other alternative.

[toddreifsteck]

New options breakdown time!! @annevk @igrigorik @DigiTec @plehegar @travisleithead

I generally prefer option 2. It keeps the scope of the API small and has a well understood surface area for both developers and implementers.

I'm going to spend Tuesday updating this PR to option 2 below unless I hear pushback.

1. Navigator.sendBeacon cannot have method added to it easily for various reasons.
   • It isn't feature detectable to add a new parameter for method that takes a dictionary. We'd have to add a type.
   • Both of the first parameters accept string so a dictionary can't be added.
   • Adding a new type for this API makes web dev programming a pain.
2. Add emitBeacon to fetch's interface. void emitBeacon(string or BeaconRequest, optional BeaconRequestInit)
   • BeaconRequest only includes method, body, headers. Everything else is hard-coded.
   • This is an expansion of sendBeacon that is still very scoped. It has a new interface and thus new members can be added and feature detected via the BeaconRequest interface.
   • Uses thrown exceptions rather than true/false like sendBeacon to remain consistent with fetch
3. Add emitBeacon to fetch's interface. void emitBeacon(sting or Request, optional RequestInit)
   • Allow fetch's full Request/RequestInit objects to be fed in.
   • Pros: No new interfaces beyond emitBeacon. Fetch code is somewhat clean to transition after dealing with new exceptions and removing the promise return.
   • Cons: The algorithm will throw in places fetch doesn't. Exposes CORs details to Beacon users which may/may not make sense. Generally more complex for a naïve user of emitBeacon. It is deceptively similar to fetch, but... the lack of promise makes it different enough to potentially catch devs off guard. The 4 CORs-related members could be a bit confusing. Cache mode is in direct conflict with the current SendBeacon's declaration of "SHOULD ignore body after the first byte".

[annevk]

I think fetchBeacon() is still a fine name since fetching is about both sending and receiving. It's a little wonky perhaps, but it does make it clearer it's related and shares fetch()' API (albeit a subset for now, as I understand from your proposal). The methods being on the same mixin does not really convey that, since mixins are not observable.

[toddreifsteck]

I'm in the midst of updating to the following WebIDL. Please let me know if there is feedback on the following WebIDL. Full spec should come in next day or two.

[NoInterfaceObject, Exposed=(Window,Worker)]
partial interface GlobalFetch {
  void fetchBeacon(USVString url, optional BeaconRequestInit init);
};

dictionary BeaconRequestInit {
    ByteString method;
    HeadersInit headers;
    BodyInit? body;
};

[annevk]

method and body should probably have a default? (That is not possible with fetch() due to the overloading of the first argument, but should be possible here.)

[igrigorik]

I'm getting a lot of inbound interest from various folks for non-POST sendBeacon. Specifically, POST is a blocker and they need support for GET/HEAD to proceed - e.g., see #22 (comment) and ampproject/amphtml#2446 (comment).

After re-reading the thread above, it seems that the PR is very close to a working solution.

That said, a quick sanity check... What are the reasons for fetchBeacon() vs simply telling folks to use fetch() with the right flags? Roughly, we're talking about:

fetch(url, {
   method: ..., 
   body: ...,            
   headers: ...,       
   credentials: 'include',
   mode: 'no-cors',
   keep-alive: true, // i.e. don't terminate when fetch group is terminated
})

Current Beacon implementations do not do any form of retries or request coalescing (which is why we dropped Age header). As such, the functional differences are:

1. Beacon sets keep-alive flag that allows the request to continue when fetch group is terminated - e.g. as page unloads.
2. Beacon rejects bodies above certain threshold (>~64KB), which is precaution for (1) to ensure that the API is not abused for large post-unload transfers.
3. Beacon doesn't provide access to the response body.

Is keep-alive flag on fetch meant as a restricted flag for internal browser APIs and we can't or are not willing to expose it to applications? If we were to add some minimal extra logic to fetch to limit body size for requests with this flag set (reject promise if exceeds), and also don't fire the promise on response body (we can still fire it on reciept of headers, that would actually be very valuable as it provides explicit feedback that beacon was processed)... would that do the trick? @annevk wdyt?

[annevk]

I think using fetch() might be okay. (See also #22 (comment).) The only question is what happens with the promise. Note that setting headers in combination with no-cors doesn't really work.

[igrigorik]

@annevk what are your thoughts on exposing keep-alive flag in Fetch to application developers? I am a little uneasy about it as I don't think we want to give applications unrestricted access to continue requests as page terminates (e.g. "oh the user is navigating away, quick start the 10mb upload of this trace file"), as this is both unintuitive to the user and also hurts the next navigation due to resource contention.

However, perhaps if we place the same restrictions around what type of requests are allowed when this flag is present, then we'd be ok? In Beacon our current solution is: check POST body size and reject if >~64KB (the limit is UA determined), also we don't allow access to response body. For fetch... If keep-alive flag is enabled:

• We can check if body is present and reject if its about some X threshold
• We can resolve the promise when headers are complete
• We can add additional step to avoid firing "process response" checks if flag is set?

[annevk]

We can resolve the promise when headers are complete

What does that mean? Request headers?

If we restrict what keep-alive can do it might be okay. Changing the semantics of the returned promise is a little icky, but not too bad I suppose. @domenic?

[igrigorik]

What does that mean? Request headers?

No, response headers, just as we do today. This is actually a plus, in that it provides an explicit ACK to the application that the request was processed by the server (if the app is still around to process the ACK, of course).

[toddreifsteck]

@igrigorik @annevk I'm 100% in agreement that we should solve this for the web.

At the same time, I'm not yet convinced that adding this to the fetch options parameter is our best option, My thinking is that I want to understand how many footguns we are handing to web developers by adding this to fetch directly.

The reason for my current fetchBeacon proposal was to attempt to avoid too many "contradictory" options members on fetch. (The obvious downside of adding a new function, fetchBeacon, is that it requires developers to learn of the existence of a 2nd networking API and effectively deprecates sendBeacon. I also believe we would not be having this entire discussion if sendBeacon were easily updated... but its signature not having an options parameters makes that impossible without potentially breaking sites).

Do we understand the full list of restrictions that would need to be added to fetch when this option is set to 'true' and the default members on fetch that a telemetry dev would need to change to use fetch successfully?

[annevk]

@toddreifsteck I think if we stick to to requiring mode: "no-cors" for now it should be okay.

On the one hand I agree that fetch() has quite a bit of complexity, on the other hand I suspect that over time the users of a keep-alive fetch API want to have similar options. E.g., custom headers and such (especially when CORS preflights get cached for an entire origin).

[igrigorik]

I agree that there are tradeoffs here, my goal in the last few messages is to make them explicit and make sure that we're all on board with whatever route we take.

On the one hand I agree that fetch() has quite a bit of complexity, on the other hand I suspect that over time the users of a keep-alive fetch API want to have similar options. E.g., custom headers and such (especially when CORS preflights get cached for an entire origin).

That's a thing already: #30. In my head...

1. If we consider keep-alive to be a private API then fetchBeacon is the only plausible route.
2. If we think that exposing keep-alive semantics on fetch is OK, then:
   1. Do we expose it unconditionally for any request? My hunch is "no", see Add method to sendBeacon #27 (comment).
   2. We add extra checks and processing carveouts for such requests, approximating Beacon.

I haven't heard any strong arguments for (1) so far. Yes, more flags on fetch is less pretty for developer ergonomics, but.. shrug, libraries and wrappers can help address that. On the other hand, giving unconditional access to (2) also doesn't seem wise, so then it becomes a question of whether limitations in (2.2) are seen as plausible and sufficient. /me looks at @annevk :-)

[annevk]

It would be easier to evaluate if someone figured out what the delta would be. But in principle if we start with exposing what sendBeacon() can do and go from there I don't really see any obvious issues.

[igrigorik]

@annevk I think the MVP is actually pretty simple...

Expose keep-alive on Request class in Fetch:

interface Request {
 readonly attribute boolean keepAlive; 
}

dictionary RequestInit {
 boolean keepAlive;
}

In the constructor...

• ~step 20: If init's keepAlive member is present, set request's keepAlive flag to it. Otherwise set it to false.
• ~step 34.4: if keepAlive flag is true, run package data algorithm (??) on body. If byte size of body is greater than XkB throw XError.

Something like the above would expose the keep-alive flag to applications, but ensure that such requests, if they have a body, are limited in the amount of (body) data they can transmit. We can keep the limit soft and let the UA's set it as they wish.

Also, on further thought, I don't think we need to touch the response processing.. If there is a response, it'll be subject to all the usual CORS bits, and there is no reason to neuter it just because keepAlive is set to true.

---

With the above, I think we have enough to explain <a ping>, sendBeacon, and probably even prefetch.

[annevk]

It'll be harder with streaming uploads. We should say something about the lifetime though, since UAs can keep the window alive if they have a bfcache or some such. Not sure if the promise should remain working that long. Also, a response from the server does not tell you the body made it all the way, unless the server guarantees that somehow out-of-band.

[igrigorik]

@annevk yep, good points. That said, I'm also reading this as "we should be able to make this work"... I'm out for the next two weeks, but I can take a run at a PR for Fetch once I'm back, unless someone else beats me to it.

[annevk]

Yeah, I was just trying to think of problems. It still seems reasonable given that keep-alive is something we expose today.

[igrigorik]

Fetch (spec) integration has landed: whatwg/fetch#388 (comment). Closing this pull request. Thanks everyone for the feedback and help!

[wanderview]

I realize I'm late to the party here, but IMO it would have been nicer to make sendBeacon() drain the Request to an internal storage space and then send it out later from there. Then the Request and its JS context don't need to stick around unless the upload body is a JS sourced stream.

[emmettbutler]

Following the trail of issues and PRs a few years later, I'm looking for some clarification on the most up-to-date recommendation. Please see this StackOverflow question.