Repository navigation
RFE: Articulate benefits of moving code to internal/... #8149
Description
Activity
/cc @nodejs/ctc, @thealphanerd, @thefourtheye, and, perhaps, @phated
Internal modules are supposed to prevent core stagnation because of userland modules (such as
graceful-fs) abusing private APIs and other implementation details.Reacted by Saúl Ibarra Corretgé, fix-fix, Brendan Ashworth, Michał Wadas and Ruben BridgewaterI think the benefit started largely as a dumping ground for stuff that was needed by core, but core did not want to officially support, like freelist. It seems to have turned into a blackhole though, which is consuming anything even remotely in the realm of things we don't want to support.
Reacted by Blaine BublitzReacted by Blaine BublitzReacted by Blaine Bublitz@isaacs The thing is — we should either state something as supported or not supported.
I don't think that we could possibly support reevaluating core node modules code, as well as monkey-patching core modules code — because either of that could break in a semver-patch version for various reasons, and there is no way to prevent that.
For monkey-patching — internal implementation details could change, breaking the monkey-patched code.
For reevaluating — the native binings API, for example, are not covered by stability level obligations, afaik, and the internal implementation could change. Also, other things are being hidden there that are not covered by stability level obligations, like internal modules, which are implementation details for core modules.
Internal modules allow us to re-use some code chunks without actually cutting them in stone and without copy-pasting those into every core module that uses them — those are shared in the
internal/dir.Reacted by Vladimir Kurchatkin, Domenic Denicola and Stephan SchneiderMoreover, I don't think that reevaluating (as well as monkey-patching) was actually supported at any point in the past, it just happened to work, and that was the cause of the whole graceful-fs issue — it used something that happened to work, but was never stated as supported, i.e. it relied on implementation details.
And now, once the graceful-fs issue is resolved completely, we won't have to worry about that anymore, and this issue won't come up again.
Ah, also /cc @jasnell and @bnoordhuis, because personal metions > team mentions.
"dumping ground" and "black hole" it is not. To be certain, very few things have actually ended up as internal modules, and the things that are there make sense being there. The distinction between internal and external is specifically around which bits of functionality need to be shared across multiple core modules but are not part of the core API. Public APIs are not supposed to change, private internal things can so long as it does not change a public API. Take
internal/bootstrap_node.jsfor instance. In general, the things that should end up in an internal module are the things end users should not be using directly.One great example of this is the
normalizeEncoding()method ofinternal/util.js. This is a utility function that is used by several other APIs in various locations in core. It is not a public facing API, nor should it be, but we do need a good way of sharing that code among the various core modules that use is. So moving that functionality intointernal/util.jsmade sense and works extremely well.The
printDeprecationMessage()API is another great example. It was never intended to be a public API. It is, however, used by multiple core modules so moving it into internal/util.js so it can be easily shared makes sense.The graceful-fs problem came up because graceful-fs chose to abuse the Node.js API. It made assumptions about the code internals that go beyond the API contract. Where Node.js chooses to keep it's internal/private code should not be a consideration for module developers so long as the external/public API contract is maintained or the changes are properly signaled (e.g. semver-major with proper deprecation)
Reacted by Dan Shaw, Drew Miller, Vaughan Rouesnel, Louis-Dominique Dubeau and Linus Unnebäckgraceful-fs chose to abuse the Node.js API
I think this is a better description:
graceful-js used undocumented elements of the Node.js API that probably should not have been exposed in the first place
Node.js has a choice here. I'm not saying one choice or the other is the right one. But we have the ecosystem we have. Implying that the fault lies with graceful-js doesn't inform the conversation. It is an unimportant detail.
Reacted by Anna Henningsen, Jeremy Whitlock, Bret Comnes and Michał WadasReacted by Dan ShawInternal modules are great for allowing smoother core development with less potential impact to the ecosystem. I don't think we could reasonably do without them.
Doing without them is like asking $module to make all of it's dependencies part of it's public API. Totally no-go in my opinion.
Sorry this broke some of your stuff, but I think it would be better for us to continue moving forward to getting past that pain.
The guiding principles of this project explicitly state that "Change just for the sake of change must be avoided" and that there's a goal of "deferring, as much as possible, additional capabilities and features to add-on modules or applications".
I don't see how the internal modules are covered in this, but this does sound like something of an argument against the WIP
unicodemodule.What value of the project is served by these sorts of architectural changes?
Maintainability. If you would like it explicitly stated on the website, I'm sure that would be possible...
while also reducing the ability to defer additional capabilities to add-on modules or applications, so I'd expect that there's some very significant benefit to make that worthwhile.
I don't understand how this would be the case. Care to elaborate @isaacs?
graceful-js used undocumented elements of the Node.js API that probably should not have been exposed in the first place
… Implying that the fault lies with graceful-js doesn't inform the conversation. …
«Fault» does not mean anything here, it is not important. According to the above, we had an issue:
API that probably should not have been exposed in the first place
It's was not an API, it was just the fact that re-evaluation was technically possible, but unsupported. Nothing was done to prevent it, but it was a reasonable assumption that it is clear enough that we just can't support it (i.e. it's stability between patch versions).
It's fixed now on Node.js side — re-evaluation throws an error, and only one module was found to be affected directly — graceful-fs, and only its old versions, as graceful-fs@4 was released more than a year ago.
Only a few modules are affected indirectly atm, and the fix path for those is straightforward — it requires only a graceful-fs version bump.
So this in fact would prevent further breakages.
Reacted by Rich Trott@Fishrock123 you broke the ecosystem. That's not okay. Gulp 3.x cannot and will not be updating graceful-fs past 3.x due to semver breaking changes. And now you are completely dropping support for a huge ecosystem player. That seems like an extremely broken stance to take.
@phated Can we patch <4.x to use the new shim or not?
you broke the ecosystem.
And we unbroke it, so now we're discussing how to move forward.
35 remaining items
there’s a reason the “evil” versions came to be how they were in the first place.
And that reason was probably just a misunderstanding on the graceful-fs side, because it turned out to be perfectly fixable on the graceful-fs side without requiring any changes in Node.js core.
Or do you mean the need in graceful-fs whatsoever and this is about embedding it's functionality in Node.js instead?
I'd like to remind that one form of "monkey patching" are polyfills - and they are not generally perceived as evil.
Even good progress and thoughtful evolution of core APIs will generate the need for polyfills for users who are not able to upgrade their core runtimes for some reason.
Therefore monkey patching (to reasonable degree and excluding protected code in
internallibs) should be supported; maybe even expected.@imyller No, polyfills for core modules should not monkey-patch generally, they should be drop-in replacements for the module instead and, perhaps, re-export things they do not affect. That is possible in most cases. See how
readable-streamandsafe-buffermodules on npm are done, for example.But that aside, graceful-fs was doing this: https://github.com/isaacs/node-graceful-fs/blob/v3.0.8/fs.js. And it wasn't for a polyfill =).
Reacted by Vladimir Kurchatkin and Sean Vieira@ChALkeR Often polyfills are needed when you don't have control over some external dependency you just need to run on top of older runtime.
Sure, you can load the drop-in-replacement like
var cm = require('coremodule-polyfill')instead ofvar cm = require('coremodule')for your own code, but core lib monkey patching polyfill allows you to support the external dependencies you reasonably can't modify to use your custom drop-in-replacement.That's all. My point was that maybe not all core lib monkey patching is evil in it's intention or implementation.
@Fishrock123 Sorry, I didn't notice your comment in the stream. This is a reply to #8149 (comment)
Internal modules are great for allowing smoother core development with less potential impact to the ecosystem. I don't think we could reasonably do without them.
Doing without them is like asking $module to make all of it's dependencies part of it's public API. Totally no-go in my opinion.
You say that like it's an obviously absurd thing, but to the extent that modules expose interfaces from their dependencies, they do treat these as part of their public API. I maintain several modules that do this. When I want to update the dep, it's a breaking change. It's kind of a pita sometimes, but doing anything else is irresponsible. (For example, breaking changes to tap-parser are considered breaking changes for node-tap. I screwed this up just a few days ago in fact, and it was a really disruptive thing for a lot of people whose tests started breaking.)
Also, the stability bar for Node.js core is much higher than it is for individual modules. That's just how it goes. A module community gets to move quickly and be somewhat fast and loose because the platform it's built on is very stable. That's the role that node core plays in this ecosystem, that's why it's a guiding principle.
What value of the project is served by these sorts of architectural changes?
Maintainability. If you would like it explicitly stated on the website, I'm sure that would be possible...I would very much like for the values of the lived Node.js project to be explicitly stated on the website. To the extent that it's stated on the website, "maintainability" is not on that list. "Stability", "empowering the community", and "minimizing changes to core" are all explicitly stated. It seems like "maintainability" is either at odds with these other values, or is a meaningless value (in the sense of "a value that no one would ever not choose, so there's no point saying it".)
If there are technical choices to be made for maintainability, then it seems like they still have to be balanced against the costs of contradicting other values, right? Otherwise why not move all the code to
lib/internalexcept just the references that you want to expose? (This is a serious question.)while also reducing the ability to defer additional capabilities to add-on modules or applications, so I'd expect that there's some very significant benefit to make that worthwhile.
I don't understand how this would be the case. Care to elaborate @isaacs?Moving code into
lib/internal/removes the ability for userland modules to get at the code, which is a reduction in their capability. You can argue that it's a worthwhile reduction, sure, but no one's saying that's not what it is; that's pretty much all that it is. Since it's a guiding principle of this project to not do that, I'm saying that there must be some other guiding principle type of value to balance out the cost.If there isn't, then it's just a bad decision, according to the stated values of this project.
At the risk of getting repetitive, this really isn't about "you broke my thing, and I'm annoyed". npm and all of my modules are upgraded to graceful-fs 4.x. I really don't care about it that much, except out of empathy for gulp, because I know that upgrading such a thing among a lot of users can be a pita.
It's about our ability in the Node.js community to predict and understand the technical choices made by the CTC. If the values being embodied by those choices aren't reflective of the values stated on the website, then it's very difficult for users trust the contributors to have their best interests in mind while making decisions affecting the overall direction of the platform. It impacts the stability and vitality of the community as a whole. I'm asking for a demonstration of an ongoing commitment to that stability and vitality.
It may seem a little picky to repeatedly demand that the stated values match the lived values of the project, but you have to understand, that's all we have to go on.
I remember back in 0.4 up to 0.10 (perhaps 0.12) that there was a guiding principle about not protecting the user. For example protecting a constant property with
Object.definePropertywas out of the question. I think this is what has changed!This I believe, started with io.js, which introduced a new set of core developers. They rebuild new a set of guiding principles (unwritten perhaps). This was from the perspective that node.js was a lot more popular and used by less experienced people, thus protecting the user became a selling point.
I don't think the "change just for the sake of change must be avoided" principle have been violated. But going from "don't protecting the user" to "protecting the user" is a change in paradigm! Thus the interpretation of that rule has changed. @isaacs I think you interpret it in the "don't protect the user" paradigm, in which case the recent changes (e.g.
lib/internal/) are meaningless and shouldn't have been made. But many other core developers interprets it in the "protecting the user" paradigm, in which case the recent changes are very meaningful.PS: I'm sorry if I missed a comment.
This issue has been inactive for sufficiently long that it seems like perhaps it should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. I'm just tidying up and not acting on a super-strong opinion or anything like that.
Just as a reminder
--expose-internalsis available for users who want to explicitly opt-in to useinternal/modules, without it being implicitly inherited byrequired modules.It is available but that does not mean that we will support anything that uses it.
Reacted by Refael AckermannYeah, it is not meant to be used by users. The flag is here for our own usage in tests and benchmarks.
I've actually been entertaining the idea of emitting a process warning when
require('internal/...')is used by any non-internal module ... with the warning making it absolutely clear that direct use ofinternal/...is absolutely unsupported.It is available but that does not mean that we will support anything that uses it.
Yeah, it is not meant to be used by users. The flag is here for our own usage in tests and benchmarks.
It's there, and it works, so we should explicitly state that use of
--expose-internalsis subject to breaking changes. IMHO it could be similar to the stetment aboutexperimentalAPIs
Ref: https://github.com/jasnell/node/blob/314b78799c5754c494b505cedd815a973c123f03/doc/api/documentation.md#stability-indexI've actually been entertaining the idea of emitting a process warning when require('internal/...') is used by any non-internal module ... with the warning making it absolutely clear that direct use of internal/... is absolutely unsupported.
One could actually have
internalpackage inside of theirnode_modules, so this is perfectly validOne could actually have internal package inside of their node_modules, so this is perfectly valid
My apologies, I wasn't clear... the warning would be emitted only if
--expose-internalsis used and the require is referencing an actual internal module.
I realize that this almost certainly has been discussed already, but could someone point me to an explanation somewhere for the benefits of moving code to be inaccessible by userland JavaScript programs?
The guiding principles of this project explicitly state that "Change just for the sake of change must be avoided" and that there's a goal of "deferring, as much as possible, additional capabilities and features to add-on modules or applications".
What value of the project is served by these sorts of architectural changes? It seems like it disrupts extant userland programs while also reducing the ability to defer additional capabilities to add-on modules or applications, so I'd expect that there's some very significant benefit to make that worthwhile.
(continued from discussion at isaacs/node-graceful-fs@07701dc#commitcomment-18677940)