Re: [whatwg/streams] Unified error handling for WritableStream (#721)

domenic commented on this pull request.

Looking pretty great... will review tests next...

>  
   assert(state === 'writable', 'state must be writable');
 
   const error = new TypeError('Requested to abort');
-  if (stream._pendingAbortRequest !== undefined) {
+  if (stream._pendingAbortRequest === true) {

Is it a boolean or a promise?

> @@ -405,7 +383,7 @@ class WritableStreamDefaultWriter {
     const state = stream._state;
 
     if (state === 'writable') {
-      if (stream._pendingAbortRequest !== undefined) {
+      if (stream._pendingError === true) {
         const error = new TypeError('Requested to abort');

Error message needs tweaking. Or should we use _storedError? I think that would be consistent with e.g. write().

> @@ -567,7 +542,7 @@ function WritableStreamDefaultWriterClose(writer) {
     stream._closeRequest = closeRequest;
   });
 
-  if (stream._backpressure === true) {
+  if (stream._backpressure === true && !stream._pendingError) {

Nit: === false to match spec text

>  
   WritableStreamDefaultWriterEnsureReadyPromiseRejected(writer, releasedError);
 
-  if (state === 'writable' || WritableStreamHasOperationMarkedInFlight(stream) === true) {
-    defaultWriterClosedPromiseReject(writer, releasedError);
-  } else {
-    defaultWriterClosedPromiseResetToRejected(writer, releasedError);
-  }
-  writer._closedPromise.catch(() => {});
+  // The state transitions to "errored" before the sink abort() method runs, but the writer.closed promise is not
+  // rejected until afterwards. This means that simply testing state will not work

Nit: period at end of sentence.

> @@ -711,8 +691,12 @@ class WritableStreamDefaultController {
       throw new TypeError(
         'WritableStreamDefaultController.prototype.error can only be used on a WritableStreamDefaultController');
     }
+    const stream = this._controlledWritableStream;
+    if (stream._pendingError) {

Nit: === true

> @@ -737,19 +720,13 @@ class WritableStreamDefaultController {
       () => {
         this._started = true;
 
-        if (stream._state === 'errored') {
-          WritableStreamRejectAbortRequestIfPending(stream);
-        } else {
-          WritableStreamHandleAbortRequestIfPending(stream);
-        }
-
         // This is a no-op if the stream was errored above.

Is this comment still accurate?

>          // This is a no-op if the stream was errored above.
         WritableStreamDefaultControllerAdvanceQueueIfNeeded(this);
       },
       r => {
-        assert(stream._state === 'writable' || stream._state === 'errored');
-        WritableStreamDefaultControllerErrorIfNeeded(this, r);
-        WritableStreamRejectAbortRequestIfPending(stream);
+        assert(stream._state === 'writable');

Maybe this assert should be in both branches now?

-- 
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/721#pullrequestreview-30212700