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.

Sign up for GitHub

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

stream: implement finished() for ReadableStream and WritableStream #46205

Merged
merged 16 commits into from
Jan 18, 2023
+301 −9
Merged

stream: implement finished() for ReadableStream and WritableStream #46205

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
fd2fa5a
stream: implement finished() for ReadableStream and WritableStream
debadree25 Jan 13, 2023
7938b4a
fixup! restore tests
debadree25 Jan 14, 2023
f0b8c92
fixup! add non-stream check
debadree25 Jan 14, 2023
3937b22
fixup! fix return
debadree25 Jan 14, 2023
70630b3
fixup! remove public property
debadree25 Jan 14, 2023
2d7ea18
fixup! use the promise and use process.nextTick
debadree25 Jan 14, 2023
b6461d9
fixup! fix code style and nullish checks remove
debadree25 Jan 14, 2023
1572862
fixup! remove nullish in writablestreams
debadree25 Jan 14, 2023
ebeab98
fixup! add tests
debadree25 Jan 14, 2023
dd5391b
fixup! remove util changes
debadree25 Jan 14, 2023
9aadb9e
fixup! lint fix
debadree25 Jan 14, 2023
e924b60
fixup! rename streamClosed to closed
debadree25 Jan 14, 2023
f1cbaea
fixup! remove nullish
debadree25 Jan 15, 2023
5ae4f8a
fixup! use symbols instead of kState
debadree25 Jan 15, 2023
4ab3a7b
fixup! remove brandchecks
debadree25 Jan 15, 2023
f1ef9e4
fixup! rename kIsClosed
debadree25 Jan 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixup! remove public property
  • Loading branch information
@debadree25
debadree25 committed Jan 15, 2023
commit 70630b330036b2fc3b6f2a08fad2b37c1d6a0403
3 changes: 2 additions & 1 deletion lib/internal/streams/end-of-stream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const {

const {
isBrandCheck,
kState,
} = require('internal/webstreams/util');
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, this will break readable-stream I think you need to inverse the dependency and have webstream require symbols from stream.

FYI @mcollina

Copy link
Member Author

Choose a reason for hiding this comment

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

As in having the symbols defined in internal/streams/util and webstreams requiring from there?

Copy link
Member

@ronag ronag Jan 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Exactly.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, we might even need to use a SymbolFor('nodejs.webstream.closed'). Similar to #45671.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok so would try to do the following, define SymbolFor('nodejs.webstream.closed') in internal/streams/util and use that instead of storing closed in the kState?

Copy link
Member Author

Choose a reason for hiding this comment

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

Have implemented this could you check?


const isReadableStream =
ronag marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -269,7 +270,7 @@ function eos(stream, options, callback) {

function eosWeb(stream, opts, callback) {
RaisinTen marked this conversation as resolved.
Show resolved Hide resolved
PromisePrototypeThen(
stream.streamClosed,
stream[kState].streamClosed,
() => callback.call(stream),
(err) => callback.call(stream, err)
Copy link
Member

Choose a reason for hiding this comment

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

You need to call the callback with a process.nextTick

Copy link
Member Author

Choose a reason for hiding this comment

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

understood updating

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed this to properly use process.nextTick

);
Expand Down
6 changes: 0 additions & 6 deletions lib/internal/webstreams/readablestream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,12 +289,6 @@ class ReadableStream {
return isReadableStreamLocked(this);
}

get streamClosed() {
if (!isReadableStream(this))
throw new ERR_INVALID_THIS('ReadableStream');
return this[kState].streamClosed.promise;
}

/**
* @param {any} [reason]
* @returns { Promise<void> }
Expand Down
6 changes: 0 additions & 6 deletions lib/internal/webstreams/writablestream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,12 +202,6 @@ class WritableStream {
return isWritableStreamLocked(this);
}

get streamClosed() {
if (!isWritableStream(this))
throw new ERR_INVALID_THIS('WritableStream');
return this[kState].streamClosed.promise;
}

/**
* @param {any} reason
* @returns {Promise<void>}
Expand Down