Implement support for JS-backed WritableStreams in JSRPC #2066
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jasnell
commented
Apr 26, 2024
jasnell
commented
Apr 26, 2024
jasnell
commented
Apr 26, 2024
jasnell
commented
Apr 26, 2024
jasnell
commented
Apr 26, 2024
jasnell
commented
Apr 26, 2024
kentonv
reviewed
May 1, 2024
jasnell
added a commit
that referenced
this pull request
May 1, 2024
removeSink is used to extract the WritableStreamSink that is owned by a WritableStream using the original implementation. It is currently used in only a few places in the codebase, none of which are necessary. There are two pending changes refactoring those two places to avoid using `removeSink()` as currently defined. Replacing the version of `removeSink` that returns the `WritableStreamSink` with a `detach()` method that returns nothing covers the sockets use case where we call the method then immediately throw away the sink. Once this and the other PRs land, we can safely remove removeSink entirely. Refs: #2061 Refs: #2050 Refs: #2066
jasnell
force-pushed
the
jsnell/support-js-writablestream-jsrpc
branch
2 times, most recently
from
58cbfec
to
ad36a3d
Compare
jasnell
commented
May 3, 2024
jasnell
commented
May 3, 2024
kentonv
reviewed
May 6, 2024
jasnell
force-pushed
the
jsnell/support-js-writablestream-jsrpc
branch
from
ad36a3d
to
5c6a775
Compare
|
@kentonv ...this should be ready for review! |
jasnell
commented
May 6, 2024
kentonv
reviewed
May 22, 2024
jasnell
force-pushed
the
jsnell/support-js-writablestream-jsrpc
branch
2 times, most recently
from
9a34e23
to
4a8a2bb
Compare
jasnell
force-pushed
the
jsnell/support-js-writablestream-jsrpc
branch
from
4a8a2bb
to
3c989ac
Compare
|
Reminder that GitHub loses information about force-pushes when you do two in a row which makes it hard to incrementally review. I think I've figured out that this diff represents the changes since I last reviewed, but in the future can you please post a list of fixup commits? The |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First pass at implementing support for JS-backed WritableStreams with JSRPC.
Provides an alternative adapter that will acquire the isolate lock on each write/close call, passing the operation off to the JS interface. This is obviously not as efficient as the original adapter used for
WritableStreamSinkbut not yet sure if there's a better approach.