stream: move legacy to lib/internal dir #8197

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
@yorkie
Member

yorkie commented Aug 20, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

stream

Description of change

This won't fix anything problem or adds any new feature, it just makes the "lib/stream.js" clear to read in my opinion, I'm unsure if this makes any sense, if the feedback from anyone in @nodejs/collaborators is negative, feel free to close :-)

@seishun

This comment has been minimized.

Show comment
Hide comment
@seishun

seishun Aug 20, 2016

Member

I don't feel qualified to judge the implementation, but +1 on the idea.

Member

seishun commented Aug 20, 2016

I don't feel qualified to judge the implementation, but +1 on the idea.

@brendanashworth

View changes

lib/_stream_legacy.js
@@ -0,0 +1,91 @@
+'use strict';

This comment has been minimized.

@brendanashworth

brendanashworth Aug 20, 2016

Member

Perhaps this belongs in lib/internal/streams/ instead?

@brendanashworth

brendanashworth Aug 20, 2016

Member

Perhaps this belongs in lib/internal/streams/ instead?

This comment has been minimized.

@yorkie

yorkie Aug 20, 2016

Member

The reason is? IMO the legacy stream class should not be internal class, it's exposed to user-land, right?

@yorkie

yorkie Aug 20, 2016

Member

The reason is? IMO the legacy stream class should not be internal class, it's exposed to user-land, right?

This comment has been minimized.

@brendanashworth

brendanashworth Aug 20, 2016

Member

That makes sense, especially since everything in the file is exported. Nevermind! :)

@brendanashworth

brendanashworth Aug 20, 2016

Member

That makes sense, especially since everything in the file is exported. Nevermind! :)

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Aug 21, 2016

Member

I think it would be better to put it in an internal file and have _stream_legacy export it as deprecated.

Member

Fishrock123 commented Aug 21, 2016

I think it would be better to put it in an internal file and have _stream_legacy export it as deprecated.

@yorkie

This comment has been minimized.

Show comment
Hide comment
@yorkie

yorkie Aug 21, 2016

Member

@Fishrock123 what do you mean by "have _stream_legacy" export it as deprecated"? Could you explain more, thanks :-)

Member

yorkie commented Aug 21, 2016

@Fishrock123 what do you mean by "have _stream_legacy" export it as deprecated"? Could you explain more, thanks :-)

@yorkie

This comment has been minimized.

Show comment
Hide comment
@yorkie

yorkie Aug 21, 2016

Member

And I will try to put the legacy file to internal dir as @Fishrock123 and @brendanashworth expected.

Member

yorkie commented Aug 21, 2016

And I will try to put the legacy file to internal dir as @Fishrock123 and @brendanashworth expected.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 21, 2016

Member

@nodejs/streams ... please take a look at this. Would moving this to internal have a detrimental impact on the readable-streams module?

Member

jasnell commented Aug 21, 2016

@nodejs/streams ... please take a look at this. Would moving this to internal have a detrimental impact on the readable-streams module?

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Aug 22, 2016

Member

@yorkie we have 3 major versions of streams: 1, 2 and 3. "legacy" is version 1, and it is used by modules such as through, split, serial-port and the like. It comes from the 0.8 era. Since 0.10 we had streams 2, and streams 3. Streams 3 are fairly stable, with their own quirks.

I think we should deprecate streams 1 as semver-major, and maybe remove it in a year or two.

I'll leave the question about readable-stream to @calvinmetcalf

Member

mcollina commented Aug 22, 2016

@yorkie we have 3 major versions of streams: 1, 2 and 3. "legacy" is version 1, and it is used by modules such as through, split, serial-port and the like. It comes from the 0.8 era. Since 0.10 we had streams 2, and streams 3. Streams 3 are fairly stable, with their own quirks.

I think we should deprecate streams 1 as semver-major, and maybe remove it in a year or two.

I'll leave the question about readable-stream to @calvinmetcalf

@mcollina

This comment has been minimized.

Show comment
Hide comment
Member

mcollina commented Dec 22, 2016

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Dec 22, 2016

Member

@jasnell @yorkie no it would not. We can move to internal without issues: there is no difference for us the moment it's on another file.

Member

mcollina commented Dec 22, 2016

@jasnell @yorkie no it would not. We can move to internal without issues: there is no difference for us the moment it's on another file.

@calvinmetcalf

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Dec 22, 2016

Member

I mean yes this would involve a bit of work to upgrade readable-stream, but it wouldn't be a big deal, honestly a drop in the bucket compared to other stuff so don't worry about it on my account.

Member

calvinmetcalf commented Dec 22, 2016

I mean yes this would involve a bit of work to upgrade readable-stream, but it wouldn't be a big deal, honestly a drop in the bucket compared to other stuff so don't worry about it on my account.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Dec 22, 2016

Member

@yorkie can you please rebase/update this PR on top of master, we can land this.

Member

mcollina commented Dec 22, 2016

@yorkie can you please rebase/update this PR on top of master, we can land this.

@yorkie

This comment has been minimized.

Show comment
Hide comment
@yorkie

yorkie Jan 6, 2017

Member

@mcollina does this PR necessary to land, current master seems not have the internal dir?

Member

yorkie commented Jan 6, 2017

@mcollina does this PR necessary to land, current master seems not have the internal dir?

@mcollina

This comment has been minimized.

Show comment
Hide comment
@yorkie

This comment has been minimized.

Show comment
Hide comment
@yorkie

yorkie Jan 6, 2017

Member

Rebased done @mcollina :)

Member

yorkie commented Jan 6, 2017

Rebased done @mcollina :)

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Jan 6, 2017

Member

@yorkie I've re-read this PR, and the majority idea is to move the legacy file into the internal folder. Can you do that as well?

Member

mcollina commented Jan 6, 2017

@yorkie I've re-read this PR, and the majority idea is to move the legacy file into the internal folder. Can you do that as well?

@yorkie yorkie changed the title from stream: move legacy to sep file to stream: move legacy to lib/internal dir Jan 7, 2017

@yorkie

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
Member

MylesBorins commented Jan 7, 2017

citgm: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/510/

edit: now with windows :D 🎉

@yorkie

This comment has been minimized.

Show comment
Hide comment
@yorkie

yorkie Jan 17, 2017

Member

Ping, and any reviews? Thanks :)

Member

yorkie commented Jan 17, 2017

Ping, and any reviews? Thanks :)

@italoacasas

This comment has been minimized.

Show comment
Hide comment
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jan 17, 2017

Member

I'm good with this but would prefer @nodejs/streams to do the review and signoff

Member

jasnell commented Jan 17, 2017

I'm good with this but would prefer @nodejs/streams to do the review and signoff

lib/stream.js
const EE = require('events');
const util = require('util');
+function Stream() {
+ EE.call(this);
+}
util.inherits(Stream, EE);

This comment has been minimized.

@mscdex

mscdex Jan 17, 2017

Contributor

Why can't we put this into the internal/* file so that the code is more exactly like what we're removing, instead of wrapping it?

@mscdex

mscdex Jan 17, 2017

Contributor

Why can't we put this into the internal/* file so that the code is more exactly like what we're removing, instead of wrapping it?

lib/stream.js
@@ -1,11 +1,18 @@
'use strict';
-module.exports = Stream;
-
-const EE = require('events');
const util = require('util');

This comment has been minimized.

@mscdex

mscdex Jan 17, 2017

Contributor

Unused.

@mscdex

mscdex Jan 17, 2017

Contributor

Unused.

lib/stream.js
-util.inherits(Stream, EE);
+function Stream() {
+ EE.call(this);
+}

This comment has been minimized.

@mscdex

mscdex Jan 17, 2017

Contributor

What I meant previously is why can't we just move this into the new internal module and export it into this file?

@mscdex

mscdex Jan 17, 2017

Contributor

What I meant previously is why can't we just move this into the new internal module and export it into this file?

This comment has been minimized.

@yorkie

yorkie Jan 17, 2017

Member

Got it :)

@yorkie

yorkie Jan 17, 2017

Member

Got it :)

lib/stream.js
+var Stream = require('internal/streams/stream').Stream;
+
+// wrap the old-style stream
+require('internal/streams/legacy')(Stream);

This comment has been minimized.

@mcollina

mcollina Jan 17, 2017

Member

This is not clear to me. Basically the definition of Stream is divided into two files, whicn is not easy to understand in my opinion. If we also move the definition of Stream, I think internal/streams/stream should also include the 'legacy' bits.

How about we move all _stream prefixed files into internal/streams? That might be a better solution overall.
cc @nodejs/streams.

@mcollina

mcollina Jan 17, 2017

Member

This is not clear to me. Basically the definition of Stream is divided into two files, whicn is not easy to understand in my opinion. If we also move the definition of Stream, I think internal/streams/stream should also include the 'legacy' bits.

How about we move all _stream prefixed files into internal/streams? That might be a better solution overall.
cc @nodejs/streams.

This comment has been minimized.

@mscdex

mscdex Jan 17, 2017

Contributor

You couldn't just move those files because that would break someone trying to require() them? Unless you mean leave behind similarly-named files that simple re-export?

Also, I think the point of this PR was to separate the actual old Stream implementation from the other things, like the re-exporting of the various stream types.

@mscdex

mscdex Jan 17, 2017

Contributor

You couldn't just move those files because that would break someone trying to require() them? Unless you mean leave behind similarly-named files that simple re-export?

Also, I think the point of this PR was to separate the actual old Stream implementation from the other things, like the re-exporting of the various stream types.

This comment has been minimized.

@mcollina

mcollina Jan 17, 2017

Member

Yes, but then why split the definition of it in one file, and the legacy part in another?
Either we just move the legacy bits, we move the full definition, or we move all the streams parts.

I'm 👎 in having both internal/streams/stream and internal/streams/legacy, if we are not moving along readable, writable, etc.

@mcollina

mcollina Jan 17, 2017

Member

Yes, but then why split the definition of it in one file, and the legacy part in another?
Either we just move the legacy bits, we move the full definition, or we move all the streams parts.

I'm 👎 in having both internal/streams/stream and internal/streams/legacy, if we are not moving along readable, writable, etc.

lib/internal/streams/stream.js
+}
+util.inherits(Stream, EE);
+
+exports.Stream = Stream;

This comment has been minimized.

@mscdex

mscdex Jan 17, 2017

Contributor

This whole file should be merged with lib/internal/streams/legacy.js

The legacy.js file should then just module.exports = Stream; and lib/stream.js should just require() that.

@mscdex

mscdex Jan 17, 2017

Contributor

This whole file should be merged with lib/internal/streams/legacy.js

The legacy.js file should then just module.exports = Stream; and lib/stream.js should just require() that.

This comment has been minimized.

@mcollina

mcollina Jan 17, 2017

Member

Just saw this, I think we agree.

@mcollina

mcollina Jan 17, 2017

Member

Just saw this, I think we agree.

@yorkie

This comment has been minimized.

Show comment
Hide comment
@yorkie

yorkie Jan 17, 2017

Member

Done :)

Member

yorkie commented Jan 17, 2017

Done :)

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Jan 26, 2017

Member

LGTM if CI and CITGM are green.

Member

mcollina commented Jan 26, 2017

LGTM if CI and CITGM are green.

@italoacasas

This comment has been minimized.

Show comment
Hide comment
@italoacasas

italoacasas Jan 31, 2017

Member

Can we land this @mcollina ?

Member

italoacasas commented Jan 31, 2017

Can we land this @mcollina ?

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Feb 1, 2017

Member

CITGM has some unrelated failures.

Member

mcollina commented Feb 1, 2017

CITGM has some unrelated failures.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Feb 1, 2017

Member

Landed as 1b30df1.

Member

mcollina commented Feb 1, 2017

Landed as 1b30df1.

@mcollina mcollina closed this Feb 1, 2017

mcollina added a commit that referenced this pull request Feb 1, 2017

stream: move legacy to lib/internal dir
Improve readability of lib/stream.js by moving the legacy abstract
Stream into lib/internal/streams/legacy.js.

PR-URL: #8197
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 2, 2017

stream: move legacy to lib/internal dir
Improve readability of lib/stream.js by moving the legacy abstract
Stream into lib/internal/streams/legacy.js.

PR-URL: nodejs#8197
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@yorkie yorkie deleted the yorkie:refine/stream branch Feb 4, 2017

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

stream: move legacy to lib/internal dir
Improve readability of lib/stream.js by moving the legacy abstract
Stream into lib/internal/streams/legacy.js.

PR-URL: nodejs#8197
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 7, 2017

Member

Requires a backport PR to land on v4

Member

jasnell commented Mar 7, 2017

Requires a backport PR to land on v4

jasnell added a commit that referenced this pull request Mar 7, 2017

stream: move legacy to lib/internal dir
Improve readability of lib/stream.js by moving the legacy abstract
Stream into lib/internal/streams/legacy.js.

PR-URL: #8197
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins added a commit that referenced this pull request Mar 9, 2017

stream: move legacy to lib/internal dir
Improve readability of lib/stream.js by moving the legacy abstract
Stream into lib/internal/streams/legacy.js.

PR-URL: #8197
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@MylesBorins MylesBorins referenced this pull request Mar 9, 2017

Merged

v6.10.1 proposal #11759

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment