Timing differences on start promise on WritableStream

[saschanaz]

What is the issue with the Streams Standard?

I was investigating further on #1296 and found that this snippet emits different logs on different implementations:

new WritableStream({
  async start() {},
  async write(chunk) {
    console.log(chunk)
  }
}).getWriter().write('write');
Promise.resolve('promise').then(console.log)

• The reference impl, Gecko, Deno: promise write
• WebKit, Blink, Node.js: write promise

UnderlyingSinkStartCallback returns any, so the IDL promise conversion doesn't happen here, and thus startResult of step 15 becomes the raw JS promise. Then step 16 wraps it with a new promise via "a promise resolved with" algorithm, and thus it takes two rounds for the listener of startPromise to be called. Promise.resolve().then() only takes one round, so the result here should be promise write, as the reference impl shows.

Instead, WebKit and Blink somehow effectively uses Promise.resolve() on the return value of startPromise: https://bugzilla.mozilla.org/show_bug.cgi?id=1859853#c1

I think we need a discussion as the implementations are divided here. I guess the spec ended up with "a promise resolved with" wrapper steps as that's what Web IDL does, but is there a real point to wrap startResult that way? What bad thing happens when we just use Promise.resolve() as WebKit/Blink/Node.js do?

[domenic]

In general it's a bad idea to use Promise.resolve()-like behavior because it results in oddities like keeping expando properties, or not converting cases like

const promise = Promise.resolve();
promise.then = doSomethingCustom;

into "normal" promises. This is why throughout the web platform we use new Promise(r => r(webDeveloperValue)) instead.

[ricea]

Yes, Blink is failing to follow the standard here. I filed https://bugs.chromium.org/p/chromium/issues/detail?id=1493934 against Blink but changing the standard might also be an option.

When writing Streams tests I have always tried to avoid counting ticks, but if this is an important interoperability issue then we'll have to start.

[saschanaz]

Thanks, Domenic!

While we are at it: the IDL has Promise conversion rule which requires creating a new promise, but when I tested with the following snippet, it seems nobody at all cares for that:

new WritableStream({
  write() {
    console.log('write')
    const promise = Promise.resolve();
    promise.then = () => console.log('then');
    return promise;
  }
}).getWriter().write()

The log in the reference impl: write then
Everyone else: write

I guess everyone is not following the Web IDL spec here. I can file an issue, but I want to double check my understanding is correct that the reference impl works right and everyone else is wrong. Am I correct here?

(A little bit more context: the write callback returns a Promise so the JS promise should go through the conversion rule.)

[domenic]

I am pretty sure the reference implementation is following the Web IDL spec and is correct.

That said, it sounds like we have multiple implementations disobeying the Web IDL spec. Not just for Streams, probably, but also for other parts of the web platform.

Although I gave a reason above why converting is generally good, it might not be worth the churn. We could just change Web IDL. I would mildly prefer aligning with the current Web IDL spec, but I would not be the one doing the work, so I'd like to see more discussion from implementers.

[petervanderbeken]

I'm going to try to align our implementation to the WebIDL spec in https://bugzilla.mozilla.org/show_bug.cgi?id=1860083.

[petervanderbeken]

One case were this is a bit weird is for PromiseRejectionEvent, creating it with a promise would end up creating an event that returns a different promise from its promise attribute.

[domenic]

Ah yep, that's weird. I guess we should just change that to object (in the spec) since it represents an opaque object reference, and doesn't want the usual promise rules applied to it.