Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove _errored and _storedError from TransformStream #800

Merged
merged 2 commits into from
Sep 22, 2017

Conversation

ricea
Copy link
Collaborator

@ricea ricea commented Sep 19, 2017

These slots are mostly redundant with the same state on the readable
side, so use the ReadableStream [[state]] and [[storedError]] instead to
reduce complexity.

It is possible for the readable to be "closed" where previously _errored
was set, but this doesn't make any practical difference as the
implementation threw a TypeError in both cases anyway.

TransformStreamError is renamed to TransformStreamDefaultControllerError to
better reflect its new semantics.

Closes #796.

These slots are mostly redundant with the same state on the readable
side, so use the ReadableStream [[state]] and [[storedError]] instead to
reduce complexity.

It is possible for the readable to be "closed" where previously _errored
was set, but this doesn't make any practical difference as the
implementation threw a TypeError in both cases anyway.

TransformStreamError is renamed to TransformStreamDefaultControllerError to
better reflect its new semantics.

Closes whatwg#796.
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some further simplifications are possible; let me know if you agree.

It sounds like tests don't change because it's a TypeError either way, right?

throw new TypeError('TransformStream is not in a state that can be errored');
}

TransformStreamDefaultControllerError(this, reason);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since TransformStreamDefaultControllerError is so short, can we just inline it here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking it might be useful as a hook for other standards, but since they are more likely to just throw from transform(), maybe not?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see, in parallel to the ones for other controllers. Yeah, maybe keep it, in which case LGTM for the whole patch.

}
}

// This is a no-op if both sides are already errored.
function TransformStreamErrorInternal(transformStream, e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably lose the -Internal suffix now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense.

@ricea ricea merged commit 569aef4 into whatwg:master Sep 22, 2017
@ricea ricea deleted the transform-stream-remove-stored-error branch September 22, 2017 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

TransformStream: remove [[errored]] and [[storedError]] slots
2 participants