| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Why should we be exposing this publicly? Is this a common use case? Is there a reference issue with more information? |
Sorry, something went wrong.
|
@mscdex I think it's a much more familiar way of creating a Duplex stream for another party, instead of having to use the push/_write/_read/_destroy/_final API. I think there'd be a great benefit if more people moved in the direction of creating a stream using a symmetrical API... see #29986 for a discussion; @addaleax suggested filing a PR over there. |
Sorry, something went wrong.
There was a problem hiding this comment.
Would anyone like to speculate why this test creates a duplex pair and then never uses the other side?
Sorry, something went wrong.
There was a problem hiding this comment.
Can we avoid adding this for new APIs? We may as well use a class, which would force new anyway.
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah I'll check it out.
Does Node.js do it this way anywhere else? I was trying to copy how the other stream classes work.
Sorry, something went wrong.
There was a problem hiding this comment.
We use class for almost everything since class has been introduced into the language. Unfortunately, we cannot convert this old-style pattern to ES6 classes without a breaking change.
Sorry, something went wrong.
There was a problem hiding this comment.
@addaleax Would it be possible to get a deprecation notice for that, then? Or is there some documentation on what causes the breaking change? (My understanding was that class is just syntactic sugar.)
Sorry, something went wrong.
There was a problem hiding this comment.
@addaleax Would it be possible to get a deprecation notice for that, then?
I don’t think this is something that we’re ever realistically going to be able to change, so there’s no real reason to deprecated it, I assume.
Or is there some documentation on what causes the breaking change? (My understanding was that class is just syntactic sugar.)
It’s the fact that classes have to be invoked with new, and will throw otherwise. That”s not the case in the current code here.
Sorry, something went wrong.
There was a problem hiding this comment.
@addaleax Thanks! I'm working on that now.
Sort of off-topic follow-up, is calling builtins without new discouraged too then? e.g. const now = Date();? I've been in discussions with a couple core members on the IRC channel, who've said more-or-less "new is obsolete these days" but I assume that was just a personal opinion around various programming styles.
Sorry, something went wrong.
There was a problem hiding this comment.
@awwright Date is special, because Date() and new Date() do different things :)
But yes, for a function that implements a pattern like this (returns an instance of itself even when called without new), I would recommend using new because that allows transitioning to classes.
Sorry, something went wrong.
There was a problem hiding this comment.
I'm trying this code:
class DuplexPair {
[Symbol.iterator] = Array.prototype[Symbol.iterator];
length = 2;
constructor() {
const clientSide = this[0] = new DuplexSide();
const serverSide = this[1] = new DuplexSide();
this.serverSide = clientSide[kOtherSide] = serverSide;
this.clientSide = serverSide[kOtherSide] = clientSide;
}
}This passes the test-release suite, but the acorn dependency can't grok it ("SyntaxError: unexpected token (42:20)" which is at the first closing square bracket). Am I missing something here?
Sorry, something went wrong.
There was a problem hiding this comment.
@awwright Yeah, we’ve run into trouble with acorn in the past for private properties as well, so I guess this falls into a similar category :/ Fwiw, I think if you do this, you could also use class DuplexPair extends Array, to automatically get length and the iterator property…
Sorry, something went wrong.
|
If we're going to make this public, I think we should be using more generic property names instead of 'client' and 'server' since it's no longer specific to tls. |
Sorry, something went wrong.
|
@mscdex I considered exposing socket1 and socket2, which is what tls uses (the test suite uses clientSide and serverSide). I settled on additionally exposing an array called pair so that you can pick any name you want: const [ socket1, socket2 ] = new DuplexPair().pair;This is what TLS now does, in the patch. Do you think that's sufficient? Some other ideas:
I think it'd be OK to expose multiple names so applications can pick one that looks most relevant. To illustrate my thought process, I think it's important to have this be a class like Duplex or PassThrough is, because I'm playing with an idea for a SimplexPair class, which is nothing more than a PassThrough with the addition of SimplexPair#readableSide and SimplexPair#writableSide properties that are only Readable and Writable, respectively. With this, you can do patterns that maintain encapsulation, like function generate(){
const stream = new SimplexPair;
stream.write("foo");
return stream.readableSide;
} |
Sorry, something went wrong.
I like this because it matches port1 and port2 from new MessageChannel(). (Alternatively: stream1 and stream2.)
Fwiw, that’s not exclusive with any naming choices. I like it. 👍 |
Sorry, something went wrong.
|
I'd prefer something even more generic than socket1 and socket2 because this interface doesn't utilize net in any way. Maybe something as simple as side1 and side2? |
Sorry, something went wrong.
|
Check my work on the iterable here: function DuplexPair() {
if (!(this instanceof DuplexPair))
return new DuplexPair();
const clientSide = this[0] = new DuplexSide();
const serverSide = this[1] = new DuplexSide();
this.length = 2;
this[Symbol.iterator] = Array.prototype[Symbol.iterator];
clientSide[kOtherSide] = serverSide;
serverSide[kOtherSide] = clientSide;
}afaict, this should behave the same as an Array, and still permit iterating over the other keys in a for..in loop. I like side1 and side2 better than anything else, except I have this hesitation because the numbers don't line up with the array indices, and I'm slightly personally attached to serverSide/clientSide, since a lot of applications will fall into this paradigm. Do we need the named properties at all, though? If performance with the iterator function is a concern, you can still do const { 0: side1, 1: side2 } = new DuplexPair;
How does this sound? |
Sorry, something went wrong.
|
@nodejs/streams Any further thoughts on this? |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
|
The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @mcollina. Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
Sorry, something went wrong.
PR-URL: #34111 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: #54123
PR-URL: #34111 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: #54123
Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: #54123
PR-URL: #34111 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
| Back | FazBrowse Home | New Git URL |
The "Duplex Pair" pattern is a function that creates two symmetrical Duplex streams, such that what is written on one side will become readable on the other.
This pattern is used by the tls module in core, and in a great number of the tests. This PR merges the two implementations together into a single stream.DuplexPair implementation.
See #29986 for a discussion of this feature.
Checklist