stream: move prefixed files into internal/streams.

[mcollina]

This simplifies the lib/ folder, and hides _stream_readable.js and
peers into lib/internal/streams. All the tests have been updated to
use the main stream module, rather than requiring from the internals.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows [commit guidelines][]

Affected core subsystem(s)

stream

[mcollina]

cc @nodejs/streams @italoacasas

[cjihrig]

Marking as semver major because this will break code that requires these files directly.

[cjihrig]

Changes LGTM with one nit.

We need to see how widely these files are required directly in the ecosystem. cc: @ChALkeR

[cjihrig]

Can you keep these with the other lib/internal files.

[mcollina]

@cjihrig done.

[jasnell]

Unfortunately we need to keep stubs in lib for these... can be deprecated exports similar to what we do with sys.js. The reason is that by removing these from the core namespace, we open a potential security risk for anyone who happens to be depending on them currently (malicious code could stick it's own _stream_readable.js or whatever into the mix and cause problems).

Moving the actual code into lib/internal is just fine, but for each deleted file, please create a lib/{filename}.js that does something like:

'use strict';

process.emitWarning('The {filename} module is deprecated. Please use ....',
                                   'DeprecationWarning', 'DEP00XX');

module.exports = require('internal/{filename}.js');

(Where DEP00XX is a new static deprecation code added to the doc/api/deprecations.md file)

[mcollina]

@jasnell how long do we need to keep stubs for?

Is it a security attack? If a malicious user can stick a file on disk, I would say the system is already compromised.

[jasnell]

Yeah, it's not a significant risk but it's still non-zero. The stubs need to remain long enough to give users sufficient time to move away from using them, which depends entirely on how extensively they are used. A quick search on GitHub for "require('_stream_readable')" returns around 14k hits where people are doing things like var Readable = require('_stream_readable'). It would take a bit of analysis to determine how critical that code might be and how much it is depended on.

[Fishrock123]

We do need to at least deprecate them. We should get some data on the usage before deciding.
I'm generally in favor if things look good though.

[mcollina]

This change make sense if we can get rid of the old files in 9 or 10. Is this the case?

[mcollina]

@Fishrock123 I do not know how to get that data.

[jasnell]

Assuming the ecosystem usage is not too high, it should be possible to remove the files by Node.js 10.0.0.

[mcollina]

@jasnell @Fishrock123 updated with a deprecation.

[jasnell]

For now, make this `DEP00XX', we can update the number when it lands, that way there's no risk of conflict

[jasnell]

Perhaps show the approved/proper way of getting these references.

[jasnell]

Generally LGTM with a couple of suggestions

[mcollina]

I would really like to have someone else from @nodejs/streams check this.

@jasnell updated.

[calvinmetcalf]

i think it looks good, we didn't allow the prefixed access in readable streams the same was, (you needed to do lib/_stream_writable) so I don't see any problem at all

[mcollina]

I will merge on Monday 27th if no one objects.

[italoacasas]

@jasnell we need to include the a Copyright in the stub files?

[jasnell]

No, the original files are moved and still retain the copyright header. The stubs are new and do not require it.

[BridgeAR]

@mcollina do you have some time soon to rebase?

[jasnell]

He's on vacation for the next couple of weeks.

[BridgeAR]

Ping @mcollina

[BridgeAR]

Closed due to no progress over a long time.

@mcollina please feel free to reopen when you get to it again.