Have each of the stream and reader hold promises #277
Conversation
| @@ -118,19 +182,11 @@ export function CreateReadableStreamEnqueueFunction(stream) { | |||
|
|
|||
| export function CreateReadableStreamErrorFunction(stream) { | |||
| return e => { | |||
| if (stream._state === 'waiting') { | |||
| stream._resolveReadyPromise(undefined); | |||
| } | |||
| if (stream._state === 'readable') { | |||
| stream._queue = []; | |||
There was a problem hiding this comment.
Any reason not to move this to ErrorReadableStream?
There was a problem hiding this comment.
Good point. Moving everything to ErrorReadableStream.
BTW, I'm trying to have some border between the code that is specific to the ReadableStream subclass implementation and one that is common to ReadableStream and all of its subclasses. Do you have any ideas about that?
|
This looks really excellent. Thank you for taking the time. Much better than the mess I was getting myself in to. I left a few minor comments but overall am very happy with this. Do you want to do the spec portion or shall I? |
|
Done addressing Domenic's comments. I also removed export from some functions in the -abstract-ops file which are not used by any other file. And, added some identity tests. May I copy the tests from your patch https://github.com/whatwg/streams/compare/cache-promises? I'm also fine if you could just commit them after mine.
I'll do but may not be able to finish today. I'll push wip one then. Please take over it. |
|
OK. Done updating the spec text. Ready for review. |
| @@ -11,17 +12,17 @@ export default class ExclusiveStreamReader { | |||
| throw new TypeError('This stream has already been locked for exclusive reading by another reader'); | |||
| } | |||
|
|
|||
| stream._readableStreamReader = this; | |||
| AttachExclusiveStreamReader(stream, this); | |||
There was a problem hiding this comment.
Any reason not to inline this? It seems to be only used in one place. Same with detach. I don't feel strongly though so if you think it's clearer we can keep it.
There was a problem hiding this comment.
I gave it its own operation name for readability. But maybe just adding comments is fine. Reverted.
|
Mostly looks good. Just a few comments, and also need to rebase on master. And yeah feel free to pull the tests, no need to keep them in a separate commit. |
tyoshino
force-pushed
the
updateBothStreamAndReader
branch
from
0f05249 to
0220345
Compare
| <li> Let <var>sourceCancelPromise</var> be PromiseInvokeOrNoop(<b>this</b>@\[[underlyingSource]], | ||
| <code>"cancel"</code>, (<var>reason</var>)). | ||
| <li> Return the result of transforming <var>sourceCancelPromise</var> by a fulfillment handler that returns <b>undefined</b>. | ||
| <li> Call-with-rethrow CancelReadableStream(<b>this</b>, <var>reason</var>). |
|
Great! Fixed a few nits and merged as 9ec484b. |
|
Thanks for committing. |
No description provided.