Invert dependency between core and readable-stream

[mcollina]

As we discussed several times in person and in WG meetings, this is the EPS to invert the dependency between Node.js and readable-stream.

cc @nodejs/streams

[yoshuawuyts]

some minor nits, overall looking 💯 - in line with prior discussions both within the WG and with TSC members

[yoshuawuyts]

typo: s/millions/million

[yoshuawuyts]

should we have backticks on readable-stream here to be consistent with prior spelling?

[yoshuawuyts]

which build script are you referring to? Perhaps being specific might be helpful (:

[yoshuawuyts]

perhaps we don't even need to mention example of tools, just the goal and generic solution could be enough

[indexzero]

+1

[lrlna]

i think this should say:

handling issues between the two repositories

the other way around sounds confusing. Apologies for not spotting it on the gist version of this EPS \o/

[lrlna]

woot woot! 🎉 ✨

[mcollina]

Adressed @lrlna @yoshuawuyts comments.

[yoshuawuyts]

Nice one

[joshgav]

nit: singular of "EP's" is "EP". "Node-EP" == "Node Enhancement Proposal"

[mcollina]

Good catch, thanks!

[jasnell]

I would add documentation to this list.

[mcollina]

@jasnell I agree. However it's a discussion to have with @nodejs/documentation, which did not happen yet.

It's listed in the "Challenges" section, meaning that after we decide this is a good thing to explore, we can engage in all the relevant discussions to make this happen.
@nodejs/streams maintain readable-stream and streams in core, and it helps with the docs, but it is not responsible for them.

If we decide this is a good idea, then we need to decide where docs should live. What do you all think about where streams docs should live?

[jasnell]

The streams docs should live with the streams project. We can maintain a copy of those with the node.js docs, but the source of truth for those should be readable-streams

[mcollina]

@jasnell I will update the EP with those point, considering also docs. I agree with you that Node.js should maintain a full copy of the docs.

[mcollina]

@jasnell the last edit should have addressed docs as well. @nodejs/streams @lrlna << please have a look

[vkurchatkin]

Is it even possible at this point? What about core using private properties?

[mcollina]

@vkurchatkin I think most of the streams modules on npm use some form of private properties of streams. I do not see any problem or differences if core keeps doing that. Most of private properties are also tested right now (see https://github.com/nodejs/node/projects/2), so we can track any possibly breaking changes.

IMHO I'm all in for documenting them and make them public, see nodejs/node#7629.

[Fishrock123]

I think it may be better to try doing this with node-inspect first. It has less possible impact and is a bit safer to try.

[cjihrig]

The plan sounds solid in principle. My biggest concern is that reporting and fixing issues could become a slower process across multiple repos, and that code review to readable-stream may not be as fast or thorough as in core (I have no experience working with the readable-stream repo, so maybe there is no concern there).

[thefourtheye]

need to be addressed

[mcollina]

thx

[mcollina]

The plan sounds solid in principle. My biggest concern is that reporting and fixing issues could become a slower process across multiple repos, and that code review to readable-stream may not be as fast or thorough as in core (I have no experience working with the readable-stream repo, so maybe there is no concern there).

A couple of notes: at the moment, the readable-stream repo is only for the "compat/browser build", so there is very little to review, a part pull from node releases. The process for landing PRs after the inversion needs to be figured out.
We (@nodejs/streams) are happy to setup a meeting a check on the process that we should follow.

Among the advantages, we can have a shorter release cycle and we can validate changes to stream well before they make into a node release. Node releases are more "immutable" than readable-stream releases.

[mcollina]

@nodejs/ctc what is the process to land this? who should accept it?

[eljefedelrodeodeljefe]

Chiming in, I think when we would make up a sum, how long processes take combined with the overhead of managing a big repo we would see that decentralised management would shorten the issue to fix lifecycle in net. Even if the e2e issue lifetime and brokerage would take longer, the overhead of aligning a lof of people and different tasks to a single process consumes a lot of unproductive time.

Issue brokerage could be a very impactful task for a bot.

[addaleax]

@nodejs/ctc what is the process to land this? who should accept it?

I guess you’d label this ctc-agenda once you think it’s in the right state for getting approval.

[mcollina]

thanks @addaleax! It would be good to have the workflow for the eps in the README.

@nodejs/collaborators what do you think about this ep? Are there any concerns that are not covered or addressed in the document?

[AndreasMadsen]

The impact of this concerns me, we have very little experience doing this level of decoupling and if done wrong it is going to impact everybody. I like @Fishrock123's idea of starting out with node-inspect.

---

some more specific comments:

• It is unclear what the responsibilities of readable-stream will be. Right now I would say (please confirm) that the responsibilities are to "port stream to older node versions and browsers", by moving stream to readable-stream that responsibility changes, but to what?

• I assume stream has quite a few V8 specific optimizations (I did a few years ago), are we going to compromise those optimizations in favor of other platforms?

• What consequences will this have for the recent and future node versions? readable-stream supports Node.js v0.8 but Node.js v0.10 just ended LTS, why should our future decisions for stream be made with thought to how we are going to support Node.js v0.8? In particular the EPS says "the latest stream from Node.js supports some features that cannot be ported to all versions of Node.js", so how can we update readable-stream without affecting older node versions?
  I realize the Process / Timeline section might hold the answer to this, but I can't follow the state of that process. The EPS should explain the finial situation in plain text, how we get to that is a secondary (but important) concern.

• ... we are currently not bumping major to avoid breaking many packages ...

Existing packages should require the current semver version, bumping the semver should thus not affect existing packages. Can you elaborate on this?

• I don't buy the documentation argument, surly readable-steam could just document those differences or alternative have its own documentation.

---

I respect readable-stream as a secondary project, but this makes readable-stream a primary project and I fail to see how this can be done without making changes in what the priorities nodejs/node currently have for stream. Maybe there are supposed to be changes, but in that case we need to document them.

[mcollina]

@AndreasMadsen thanks for your comment, I took some time to reply to this, sorry it would be a long reply.

The impact of this concerns me, we have very little experience doing this level of decoupling and if done wrong it is going to impact everybody. I like @Fishrock123's idea of starting out with node-inspect.

I agree. We can leave this as pending for the time being, and when that node-inspect decoupling happens we can move this forward. I'm happy to be part of the team that does the decoupling, so I can bring forwards some of the lesson learned.

It is unclear what the responsibilities of readable-stream will be. Right now I would say (please confirm) that the responsibilities are to "port stream to older node versions and browsers", by moving stream to readable-stream that responsibility changes, but to what?

Ideally support streams in older versions of Node and browsers. Streams are an overall compatibility layer that enable a very wide ecosystem of modules to work mostly everywhere.
I will update the proposal to reflect this.
Very recently it was asked to support streams in React Native. I think that is a valid stretch to our goals.

I assume stream has quite a few V8 specific optimizations (I did a few years ago), are we going to compromise those optimizations in favor of other platforms?

At the moment the battle is V8-now vs V8-old. I do not see making streams slower as something that could happen anytime soon. The whole wg is invested in making streams faster in Node.js and not breaking compatibility. If we can improve the performance in other platforms without compromising Node, we will. At this point, our only benchmarks are in Node.js.
There is also node-chakracore coming.

We can add that sentence to the Streams WG description and responsibility, if you think we should make it official.

What consequences will this have for the recent and future node versions? readable-stream supports Node.js v0.8 but Node.js v0.10 just ended LTS, why should our future decisions for stream be made with thought to how we are going to support Node.js v0.8? In particular the EPS says "the latest stream from Node.js supports some features that cannot be ported to all versions of Node.js", so how can we update readable-stream without affecting older node versions?
I realize the Process / Timeline section might hold the answer to this, but I can't follow the state of that process. The EPS should explain the finial situation in plain text, how we get to that is a secondary (but important) concern.

We are already monkey-patching things quite a bit in readable-stream. This will not change, as we will target the latest Node.js as the primary development platform. There are two place where this is currently happening, and we handled both without breaking 0.8, 0.10 and 0.12 support. Eventually we will drop them as they fade out of usage.

I will update the EP to reflect this.

... we are currently not bumping major to avoid breaking many packages ...
Existing packages should require the current semver version, bumping the semver should thus not affect existing packages. Can you elaborate on this?

If we bump the major, there will be a lot of modules to update (1308), and they will do it on their own time. This will cause a lot of the ecosystem to be upset, because they will likely have two or three versions of readable-stream in the tree. And the community is already complaining about readable-stream size (there is a lot of story there, and I do not want to sidetrack this discussion if it is not of interest).

I don't buy the documentation argument, surly readable-steam could just document those differences or alternative have its own documentation.

Can you clarify this point?

I respect readable-stream as a secondary project, but this makes readable-stream a primary project and I fail to see how this can be done without making changes in what the priorities nodejs/node currently have for stream. Maybe there are supposed to be changes, but in that case we need to document them.

What are the priorities Node has for streams?
I disagree on the fact that readable-stream is a 'secondary' project. readable-stream underpins most of the ecosystem around streams. It makes sure things works the way they should across node versions, different frameworks and even package managers. This is secondary compared to Node.js, but we have to consider a lot more things than Node.js, e.g. we broke yarn for 25 minutes last year.

[AndreasMadsen]

Can you clarify this point?

It is referring to:

At the moment readable-stream does not have its own documentation, it is the
documentation of whichever version of Node.js is built from. This creates confusion,
as the latest stream from Node.js supports some features that cannot be ported
to all versions of Node.js.

---

Could you explain why we need readable-stream in the first place, not just for older node version and the browser but also for the latest node version?

[mcollina]

I have updated the proposal, clarifying some points as we discussed.
@yoshuawuyts @mafintosh please check again :).

@AndreasMadsen I will add some more clarification about what role readable-stream fullfill.

[mcollina]

@AndreasMadsen I've added an explanation on why readable-stream is important on the EP. Let me know if you have more questions.

[mcollina]

@nodejs/ctc we are ready to discuss this, can someone put the ctc-agenda label on this?

[mhdawson]

At the end it says "We think we should target this to be shipped in Node 8." is that still the goal ? It seems pretty close to our target ship date for Node version 8.

[mcollina]

@mhdawson not really. This got stuck for a while, consider it Node.js 9.

[rvagg]

Discussed again in CTC meeting. Nothing really new although I voiced my support of this switch of responsibility & authority. It seems like it should probably be removed from the CTC agenda until it requires a vote? Perhaps leave on ctc-review unless @mcollina feels that discussion has been exhausted and it needs to be resolved with a vote.

[sam-github]

Fwiw, I think this makes sense. The readable_streams group is Node.js WG, they are not going to compromise the performance of Node.js-latest in order to support old Node versions, and this will aid their maintenance. Sounds good to me.

[mcollina]

Feel free to move this to ctc-review, as I do not have management rights on this repo.
I'll come up with a couple of example PRs, and see how things might work.

[Trott]

Closing, but of course comment or re-open if closing isn't right.