w3c / webtransport

WebTransport is a web API for flexible data transport
https://w3c.github.io/webtransport/
Other
845 stars 50 forks source link

Expose [[State]] internal slot #539

Closed achingbrain closed 6 months ago

achingbrain commented 1 year ago

The spec sets out a [[State]] internal slot that contains the current state of the transport.

It would be really useful to expose this as an attribute to allow js code to have a little more introspection ability.

It would help with application observability and diagnostics, and also to better understand the lifecycle of a WebTransport object.

aboba commented 1 year ago

This seems duplicative. We already have the ready promise and errors. If we added this, we'd need a corresponding event handler (onstatechange). The state change events would correspond to the ready promise and errors. So it's not clear to me that the application would obtain more information.

jan-ivar commented 1 year ago

I agree it's duplicative since we went with promises over events and state. Here's a polyfill that works:

if (!('state' in WebTransport.prototype)) {
  const nativeWebTransport = window.WebTransport.bind(window);

  window.WebTransport = function(url, options) {
    const wt = new nativeWebTransport(url, options);
    wt.state = "connecting";
    wt.ready.then(() => wt.state = "connected", () => wt.state = "failed");
    wt.closed.then(() => wt.state = "closed", () => wt.state = "failed");
    wt.draining?.then(() => wt.state = "draining", () => {});
    return wt;
  };
}
jan-ivar commented 9 months ago

I propose we close this at next meeting.

achingbrain commented 9 months ago

The problem with the suggested polyfill is that if you call .then with two args, if the promise rejects it no longer causes an unhandled promise rejection.

If a user wants this in their application that's down to them but it's not the sort of code you could reasonably include in a library.

Exposing the [[State]] slot would allow observability without changing the behaviour of the thing it is observing.

jan-ivar commented 9 months ago

@achingbrain thanks for spotting that. Rethrowing the error should hopefully fix that.

if (!('state' in WebTransport.prototype)) {
  const nativeWebTransport = window.WebTransport.bind(window);

  window.WebTransport = function(url, options) {
    const wt = new nativeWebTransport(url, options);
    wt.state = "connecting";
    wt.ready.then(() => wt.state = "connected", e => { wt.state = "failed"; throw e; });
    wt.closed.then(() => wt.state = "closed", e => { wt.state = "failed"; throw e; });
    wt.draining?.then(() => wt.state = "draining");
    return wt;
  };
}
jan-ivar commented 9 months ago

Hmm, that might always trigger unhandled rejections. I'll have to think about it a little more.

aboba commented 9 months ago

@jan-ivar I agree with closing this issue. So far, I haven't found a situation in which a [[State]] internal slot would be useful.

achingbrain commented 9 months ago

So far, I haven't found a situation in which a [[State]] internal slot would be useful

Here's one - I put this demo page together while investigating a bug that makes WebTransport unusable for peer to peer applications, it tracks the state of various WebTransport connections opened by the page.

It does so by doing something similar to the suggested polyfills above, but due to the semantics of using promises for .ready, .closed, etc it can't observe the state of a connection without changing it's behaviour.

[[State]] would have been very useful here.

Here's another, though similar - I'd like to also expand the observability of js-libp2p through a dashboard similar to ipfs-webui but again this isn't something we can do without either causing unhandled promise rejections or smothering errors.

jan-ivar commented 8 months ago

Meeting:

jan-ivar commented 6 months ago

Meeting:

saschanaz commented 6 months ago

Streams has several "Set promise.[[PromiseIsHandled]] to true" in several places, is that what you want?

saschanaz commented 6 months ago

Briefly checked other existing readonly attribute Promises and those never reject, while WebTransport rejects promise attributes. 👀

jan-ivar commented 6 months ago

I found streams rejecting reader.closed here.

[[PromiseIsHandled]] might work.

wilaw commented 6 months ago

@achingbrain - we have discussed this over the last 3 bi-weekly meetings. The groups conclusion was that we prefer to maintain consistency with the Streams spec in not exposing state and that a polyfill solution for state management was preferred. You correctly pointed out the problem with unhandled promised rejections in imple,menting that approach. To that end, we have opened #601 to fix that issue. We appreciate your raising of this issue and hope that post-601 fix, a polyfill approach will meet your needs.

achingbrain commented 6 months ago

My opinion is that libraries should not modify built ins or globals as the results can be unpredictable when deployed in an app with other libraries, so I'd rather not go down the polyfill route but it that's the only option then that's what it's going to have to be.

Thanks for looking into this.

jan-ivar commented 6 months ago

With the polyfill I only meant to suggest these ergonomics can be provided by a JS library. This can be done as a function.