Re: [whatwg/streams] WritableStream abort logic clean up (#655)

domenic commented on this pull request.

Answering the Qs:

> Q: Is this "one pending write at a time" restriction a part of WritableStream's semantics or WritableStreamDefaultController's semantics?

I don't have a strong opinion on this.

> Q: Do you like this new behavior?

This seems correct to me.

---

Overall this is a big change, and it will be helpful if you could draft the commit message explaining all that it changes and what the benefits are. The OP has some of the non-public facing changes but it sounds like there are also some changes to the behavior during error conditions.

I also wonder how this will tie into https://github.com/whatwg/streams/pull/634, both the normative change there and the suggested style change to use the queue more.

> +  const ws = new WritableStream({
+    close() {
+      return Promise.reject();
+    }
+  });
+
+  // Wait until the WritableStream starts so that the close() call gets processed.
+  return delay(0).then(() => {
+    const writer = ws.getWriter();
+
+    const closePromise = writer.close();
+    const abortPromise = writer.abort();
+
+    const events = [];
+    return Promise.all([
+      closePromise.then(() => {

I would redo these as

```js
promise_rejects(t, new TypeError(), error1,
  'closePromise must reject with the error returned from the sink\'s close method')
.then(() => events.push('closePromise'))
```

Same for all tests. In general using assert_unreached for promises seems like a code smell.

> @@ -529,6 +530,242 @@ promise_test(t => {
 }, 'writer.ready should reject on controller error without waiting for underlying write');
 
 promise_test(t => {
+  let rejectWrite;
+  const ws = new WritableStream({
+    write() {
+      return new Promise((resolve, reject) => {
+        rejectWrite = reject;
+      });
+    }
+  });
+
+  let writePromise;
+  let writePromiseRejected = false;

In general I prefer arrays of events and assert_array_equals to boolean flags, but it's not a big deal.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/655#pullrequestreview-17110046