iojs: introduce internal modules

[vkurchatkin]

Internal modules can be used to share private code between public modules without risk to expose private APIs to the user.

Later some flag (build time or runtime) could be added for making unit-testing possible.
#569

R=@iojs/owners @iojs/collaborators

[seishun]

Does this actually help? Crazy people will just require('internal/freelist') in their code.

[vkurchatkin]

that's the idea, they can't. require('internal/whatever/module) works only inside built-in modules. For users everything works as before. That is, if node_modules/internal/whatever/module.js exists, they will get it. Otherwise the will get an error

[seishun]

Ah, I see now. I'm not sure if it's worth introducing such a framework for just one module which might get removed.

[vkurchatkin]

It is not just for one module. The idea is to move there everything non-public bit by bit. freelist is the least of mine concerns, because it's not actually shared and could be embedded into _http_common. freelist was just chosen for illustrative and testing purposes.

[thlorenz]

tl;dr if you use undocumented modules you run the risk of your code breaking, no need for us to fix anything here.

Not a big fan of this feature.
If someone uses things that aren't documented then there is a known risk that that code won't work in future versions.
This is true for a lot more things than modules, i.e. functions on prototypes that aren't part of the API but used internally. Usually those are marked as _functionName.

As an example inside readline-vim I depend on readline._ttyWrite but only consider myself lucky that the code didn't break due to the function name changing. However I knew that risk when I wrote that code.

Same is true for any modules only to be used internally. If you wanna use them, fine but just know the risk. But if you start trying to encapsulate those you'd then also have to encapsulate the _private functions.

From my experience in other OO environments I've come to the conclusion that encapsulation is overrated. Just let the users make the right decisions. Marking things private explicitly or implicitly (by not documenting them) should suffice.

[brendanashworth]

I'm unsure about this change. Yes, developers using private modules causes hesitancy when putting breaking changes into the source and not bumping semver-major, but we have to draw the line somewhere - and I think that should be whether or not it is documented. The programmer should know what they are doing, and if they know what they are doing and want to require a private module, we shouldn't restrict them from doing that.

[vkurchatkin]

What you are saying is absolutely right, but also purely theoretical. When time comes to remove something that was never documented we still HAVE TO evaluate the risks of removing to it. This is the burden I want to get rid of.

However I knew that risk when I wrote that code.

You know the risk, someone doesn't. Someone who requires you module doesn't. Removing this method can potentially break thousands of modules and the only person who knew the risk was you.

But if you start trying to encapsulate those you'd then also have to encapsulate the _private functions.

Not HAVE TO. But this would be good too, eventually.

Marking things private explicitly or implicitly (by not documenting them) should suffice.

This is exactly that, marking modules as private very explicitly.

[thlorenz]

Not sure if it was clear, but I meant to stress implicitly (by not documenting them) should suffice above, meaning that no further action beyond not documenting is needed.

You know the risk, someone doesn't.

When I wrote that code I wasn't involved with node myself, but knew as a developer that undocumented features and/or functions marked _ are considered private API and I'm running a risk when using it.

Most devs are aware of this as I was and if not they'll have to learn the hard way unfortunately. However jumping through hoops just to somehow disable devs not to follow these conventions is over the top IMHO.

[vkurchatkin]

While you are right (as I have already said), what you say is unrealistic and unreasonable.

Let's say _ and not documenting is enough. So let's remove util._extend. I bet half of npm would break. It can be done in one line, there numerous libraries on npm, and people still use it.

It's so easy to discover something not documented using REPL. root instead of global? cool I'll use that!. process.EventEmitter? cool, one require less. And so one. People who do that are asking for trouble, no doubt. But what about people depending on their modules? They do nothing wrong.

I understand your stance, but you give no cons. Here are the pros (from my point of view):

• true privacy, that what people always wanted in javascript, but had to go with _;
• encourages code sharing in lib;
• encourages modularity in lib- we can have small files, deep structure, etc. (/lib/internal/http/parser.js)
• reduces risks or refactoring.

Some references:

https://github.com/search?l=javascript&q=util._extend&ref=searchresults&type=Code&utf8=%E2%9C%93
#669
#825
#766
nodejs/node-v0.x-archive#6621
nodejs/node-v0.x-archive#8533

[meandmycode]

I'd agree that it's io.js / node that pay for other developers risks. Especially with how graph like dependencies are in node it's easy for one person's risk to affect hundreds without them realizing. In other languages it's a lot harder to shoot yourself in the foot (.NET for example requires you write reflection code and a level of execution permission that people don't assume).

This will always be a balance but if one of io.js goals is agility then it seems to fit.

[thlorenz]

I agree that the convention WRT _ private functions were not followed consistently which doesn't mean we have to steer away from those conventions completely.

But let's get the discussion back on track to what this PR is about.
Here is my opinion and it'll be my final comment in this thread (hopefully others will add theirs so we can come to the right decision here):

Internal modules to prevent users from doing things that may cause problems for them would be nice and if they came at no cost I'd be all for it. However the price to pay for this (added complexity in the code base and possible confusion about what's going on for people trying to grok the code base) is just not worth it IMO.
So I'm a clear 👎 on this.

[tellnes]

I'm generally in line with @thlorenz points in this thread. But if we should implement something like this, I don't think creating a magic internal folder is the way to go. I think a better solution would be to create a list of the modules which should be exported similar to how we are defining what should be exported in modules by adding a property to module.export.

[seishun]

-1 for reasons already outlined by @thlorenz and also because it doesn't solve the problem completely anyway. There are still undocumented object properties and undocumented exports on otherwise public modules.

[Qard]

Indeed. Nice try, but only a partial fix.

Also, I would've gone the route of freelist being in a node_modules folder only resolvable from other core files, so it could just work like any other module dependency not being accessible without doing require('parent/node_modules/dependency').

[chrisdickinson]

I don't have time at the moment to fully write this up at the moment, but I actually would like to have this ability in core.

[brendanashworth]

For the sake of the functionality (regardless of my stance), why not just block require's starting with an underscore rather than the folder?

i.e.:

> require('_http_common');
TypeError: cannot require private module
    at repl:1:7
    ...

[tellnes]

+1 for underscore in favor of internal/

[vkurchatkin]

For the sake of the functionality (regardless of my stance), why not just block require's starting with an underscore rather than the folder

@brendanashworth that would be an immediate breaking change. The idea is to leave underscored files and add deprecation notice to them until 2.0.0. Also a part of the PR is enabling subfolders in `lib/', which is also nice to have.

There are still undocumented object properties and undocumented exports on otherwise public modules.

@seishun They are on my radar too, but this is a first essential step. If we want to move private stuff, we need a place where to move it first.

[trevnorris]

We should also consider, as best we can, the long term effects with ES6 modules.

[chrisdickinson]

This is a great example of a place where I'd love to be able to have modules private to io.js – it would be great to have an internal module that allowed io.js internals to place "high priority" event listeners, vs. reaching into other object state.

[benjamingr]

When I started reading it sounded like a bad idea but I think @vkurchatkin makes some really good arguments in favour encapsulation here.

In many languages it's possible to use modules internally but not expose them to the outside. This isn't only a problem in core, it would also be a nice ability in userland to signal that a specific file from a module should not be included directly. Popular userland libraries share the issue.

Something like a:

module[Symbol.for("internal")] = true;

Which would mean this file should not be included from the outside - only from within the module or core. It's not a well thought out idea but I hope it gets the point across.

I also think that @trevnorris's comments about ES6 modules are really important - any change here can have an impact on ES6 modules that don't do this sort of thing.

[brendanashworth]

@brendanashworth that would be an immediate breaking change. The idea is to leave underscored files and add deprecation notice to them until 2.0.0. Also a part of the PR is enabling subfolders in `lib/', which is also nice to have.

How so? I think it all could be done the same way - issuing a deprecation notice when they are require'd via the module code. It could then be changed to throw an error on 2.0.0.

The change for enabled subfolders could also be separated into a new commit.

[benjamingr]

@yursha because being able to require parts of a library or a module is super useful and a lot of packages use that ability.

[vkurchatkin]

I also doubt that its a wise idea to implement modules differently in core and in the userland

They are already implemented differently. There are many reasons for that, both historical and practical.

[vkurchatkin]

@yursha Also a problem, but much harder to solve. If we prohibit require('some_module/lib/internal.js') people still could use require('../node_modules/some_module/lib/internal.js')

[tellnes]

@yursha Internal and userland modules are a different story. Your feature request to support this for userland modules is a real one, but out of scope for this pull request.

If you can come up with a good and clean way which make it possible to require parts of a module like it works today and at the same time lets you define some files as unrequireable, then you maybe have a solution.

[tellnes]

@yursha Like @benjamingr said. It is super useful. For an example, look at lodash.

[vkurchatkin]

I'm just trying to understand why we need requiring parts of the module in the first place.

Useful for browserify. Otherwise index.js with explicit exports is fine

[vkurchatkin]

@yursha browserify includes only files that are explicitly required. So if you require index.js it will include everything, while you might need only a couple of files

[Fishrock123]

This commit causes the entire windows test suite to fail due to some config loading issue: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/381/nodes=iojs-win2008r2/console

node.js:275
    config = config.split('\n')
                   ^
TypeError: Cannot read property 'split' of undefined
    at Function.startup.processConfig (node.js:275:20)
    at startup (node.js:32:13)
    at node.js:889:3

[Fishrock123]

I don't really understand how it could cause that, but the parent commit works fine, and this ones throws like crazy.

[bnoordhuis]

I'll look into it. I think I know what's causing it and I guess I'm partially to blame for lgtm'ing without pointing out the CI failures.

[vkurchatkin]

Looks like gyp passes paths to js2c.py differently on windows

[bnoordhuis]

FTR: #1281