Node: RFE: Articulate benefits of moving code to internal/...

[isaacs]

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 https://github.com/isaacs/node-graceful-fs/commit/07701dc98bc088e1a71f18a8f59a970e3354646f#commitcomment-18677940)

[ChALkeR]

/cc @nodejs/ctc, @TheAlphaNerd, @thefourtheye, and, perhaps, @phated

[vkurchatkin]

Internal modules are supposed to prevent core stagnation because of userland modules (such as graceful-fs) abusing private APIs and other implementation details.

[Qard]

I 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.

[ChALkeR]

@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.

[ChALkeR]

Moreover, 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.

[jasnell]

"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.js for 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 of internal/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 into internal/util.js made 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)

[Trott]

graceful-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.

[Fishrock123]

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.

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 unicode module.

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?

[ChALkeR]

@Trott

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.

[phated]

@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.

[Fishrock123]

@phated Can we patch <4.x to use the new shim or not?

[Fishrock123]

you broke the ecosystem.

And we unbroke it, so now we're discussing how to move forward.

[phated]

@Fishrock123 @TheAlphaNerd attempted it (in the linked commit) but I guess there are even more breaking changes that can't be backported. Node core shouldn't actively be breaking the ecosystem at these levels. I understand that some changes need to be made, but extra care needs to be taken for the ecosystem at large and not just throw the proverbially hands up.

[ChALkeR]

@phated

you broke the ecosystem. That's not okay

@bnoordhuis fixed graceful-fs more than a year ago. I introduced the deprecation message over half a year ago and I also have been personally notifying various ecosystem modules since then.

What else could be done here?

We can't just stop moving and be back to «don't break anything, let's keep v8-v-three-and-something forever». Also note that this is not some changes in documented API, all of this is just due to a misuse by graceful-fs.

[vkurchatkin]

And now you are completely dropping support for a huge ecosystem player.

That' where you are wrong. You can't assume that every package on npm is supported and hiding internals is the way to make sure that if a package works now, it will work in the future.

[phated]

@ChALkeR native modules are a different beast than the JS library code. I've seen way....too...many twitter rants in the past few months about node core breaking stuff for the sake of breaking stuff (non-blocking stdout, graceful-fs, removing Object.prototype inheritance). This stuff is bordering on the insane and wastes too many people's time.

The implications of upgrading graceful-fs are very extreme for us and effect every consumer. It's not just an interop problem for gulp because it changed the runtime environment for every end consumer. It will undoubtably break some portion of consumers and I am not willing to do that in a non-major bump and unfortunately we aren't ready for a major bump yet. In our in-progress major bump, graceful-fs is already updated, but even after the bump, consumers have to rewrite their gulpfiles which is a large undertaking for some of them. I'm sorry we can't all move as fast as node core 😖

[Fishrock123]

Sorry, "shim" is probably not the correct word, but I _currently_ do not see why graceful-fs v4.x's wrapping mechanism could not be used in both graceful-fs v3.x and v2.x. As far as I can understand, that should not be breaking and should be possible with some effort (if necessary I _may_ be able to help).

(Again, we are not currently breaking you and this is still in discussion.)

[Fishrock123]

non-blocking stdout

oh please don't bring that up like that, no-one even knew that was coming because of a bug layers deep that no-one thought about and were never any relevant tests for. Would you have thought about it?

[ChALkeR]

@Fishrock123 that makes no sense ­— that change was the only reason for graceful-fs to bump it's major version. If we do that, the next obvious step for gulp, following its current path (not wishing for this change in gulp 3.x), would be to bind gulp 3.x to a non-fixed semver-patch graceful-fs version without this change.

[ChALkeR]

@phated Btw, were you able to reproduce any issues with just upgrading to graceful-fs@4?

The only reason for the major graceful-fs bump was this fix, and the 2.x and 3.x versions of graceful-fs are horribly broken. They are even documented to be horribly broken! graceful-fs@4 was fixed by @bnoordhuis to be sane.

The only non-backwards-compatible situation that I could come up with with upgrading from graceful-fs@3 to graceful-fs@4 is when something else monkey-patches (ew) the fs module, graceful-fs@3 uses the original version (by re-evaluating core fs module sources), and graceful-fs@4 would use the module that has been tampered with. Do you have anything else in gulp which monkey-patches fs so badly that it would break in this situation? If not, I don't see any issues with upgrading gulp@3 to use graceful-fs@4.

I believe @TheAlphaNerd did some tests, and all of those passes on gulp@3 + graceful-fs@4. @TheAlphaNerd , is that correct?

[ChALkeR]

We could just solve all issues here if gulp@3 just upgraded to graceful-fs@4. Moreover, in the (extremely unlikely) situation if something bad happens — you could always revert, and that is one of the reasons why doing this in advance is good, before we landed the actual breaking changes here.

[phated]

@ChALkeR the problem is that the entire environment is changed in an unquantifiable way. The gulp and vinyl-fs test suites were not up to par that far back, but even still the change doesn't just effect our code, it effects everyone that wrote a gulpfile. I've seen gulp 3.x gulpfiles that are thousands of lines long and there's no way to review/test/audit them all.

Every time we try to change even a tiny thing in gulp 3.x, it breaks people. That's why it has been frozen for a long time.

[phated]

@ChALkeR you aren't solving "all the issues here" because there's a fundamental miscommunication in how node core handles breaking changes in the JS-portion of the standard library. That's the whole reason for the issue (not the gulp problems that exacerbate it).

[ChALkeR]

breaking changes in the JS-portion of the standard library

That's not a breaking change in the JS API, it's module reevaluation. Imagine if someone was searching for the gulp directory and reading package.json of gulp, getting some info out of that (e.g. the description or the version number itself), used that to build stuff upon, then came to you complaining that their package broke when you upgraded yours.

The API wasn't broken or even affected by anything of this.

[phated]

@ChALkeR you own every part of the API, everything that is possible. Any breaking change to any consumer is a breaking change on your part and should be weighed as such. I gave a talk on this very thing at Thunder Plains last year. In which I said (and is a good example): "If you export an underscored property and someone uses it, removing it is a breaking change". As a runtime, this should be an even more restricted area of change because the entire ecosystem based around you.

[vkurchatkin]

If you export an underscored property and someone uses it, removing it is a breaking change

And that's why we have internal modules now. But still, this has nothing to do with graceful-fs

[ChALkeR]

@phated It's not even an underscored property. Re-evaluation is not a part of the API. The closest example I could come up with is someone searching for unrelated non-exported gulp files, like package.json, and having some assumptions that that file would never change, even between major versions. Yes, it's exactly as crazy as it sounds.

[phated]

@vkurchatkin this thread isn't about graceful-fs in particular. It's about your decision to break part of the ecosystem due to wanting to change to internal/* and how that decision is restrictive to the ecosystem (both past and future) and limits what types of modules can even be written. None of which has really been addressed yet.

[phated]

@ChALkeR vm.runInContext is part of the API, so I would argue otherwise.

[vkurchatkin]

@phated we didn't (and don't) remove underscored properties without deprecation. graceful-fs broke because it does a crazy thing. It's so obviously crazy, that it's not even worth arguing about.

[ChALkeR]

@phated vm.runInContext has nothing to do with it, it could have been eval, it's just a tool used for re-evaluation.

It's like saying that the abovementioned wonderful package.json technique should be supported because I could have used only the supported fs module, and, for example, some supported math operators to calculate the CRC of the description and break if that changes (e.g. use it as a salt for my passwords).

[addaleax]

graceful-fs broke because it does a crazy thing.

Well, if we don’t want to force people into monkey-patching Node internals, we need to provide proper APIs for the things they are trying to do ourselves. (Also, as a very general rule, I can only suggest strongly to be very careful throwing around words which are also commonly used to insult people.)

[ChALkeR]

@addaleax graceful-fs was rewritten without monkey-patching by @bnoordhuis more than a year ago, and nothing is wrong with the new version. Only the old version is evil.

That automatically means that the users weren't _forced_ into monkey-patching, it's just old graceful-fs was doing the wrong thing.

[phated]

@ChALkeR again, assuming the ecosystem can or will move as fast as node (for some reason) wants to move is a really bad attitude to have.

[addaleax]

@ChALkeR Yeah, I’m just saying… there’s a reason the “evil” versions came to be how they were in the first place.

[jasnell]

I would ask everyone to please take a breath and hold up a sec. This conversation is not being very productive nor is there evidence that it's heading that direction.

The point here is this: Node.js Core needs to be able to have internal APIs that are hidden from the module ecosystem, we also need to have the ability to make necessary internal changes so long as we are adhering to our API contract and semver-rules. We also need to make sure we're being sensitive to the needs of the ecosystem.

That said, there are several possible constructive paths forward.

First, the CTC discussed this today and while there was some disagreement, we generally landed on holding off making further internalizing changes to the fs module until better solutions arise that do not break anyone. We will revisit that decision possibly before v7 is cut and _definitely_ before v8 is cut next April. That gives us at least 6-7 months to play with to figure out a better approach.

Second, several people (primarily @TheAlphaNerd) are looking at various alternatives. One idea that I had (that I want to investigate further) is whether it would make sense to have a separately installable fs module, managed by core, that would be similar in nature to readable-streams. This would be maintained in parallel to core, but with an implementation that ensures that it is standalone, so that when we need to make breaking changes, they can be reasonably ported to a standalone module that can be handled as a regular dependency. It doesn't solve all of the problems, but it would provide a reasonable buffer (I believe).

Third, for the fs module specifically, we should organize a @nodejs/fs review team that includes a mix of core collaborators and fs-related module developers to review changes being made to the fs module, with an emphasis being put on making sure changes are being extensively reviewed.

[bnoordhuis]

I've seen gulp 3.x gulpfiles that are thousands of lines long and there's no way to review/test/audit them all.

I have to admit I don't really understand what you're afraid of but I don't want to derail this thread further.

Can you open a new issue and post an outline of where you think the danger in upgrading graceful-fs is and what node core can do about that?

[phated]

@bnoordhuis I agree on avoiding the derail but there's nothing to open. Our change is already in progress but can't be backported safely.

[ChALkeR]

@bnoordhuis For the record, I proposed a way to fix this on the gulp side in May: https://github.com/gulpjs/gulp/issues/1640 — and I still fail to see how that change could possibly affect anyone, except for the unlikely situation that I described above: https://github.com/nodejs/node/issues/8149#issuecomment-240547322.

[ChALkeR]

@jasnell Another question — for how long would we want to block this?

Gulp provides more than 40% of the current graceful-fs@[23] users (counting @latest versions only, as there is no per-version stats coming from npm), with the other ones being just some old modules like unzip which weren't updated for several years and whose usage is falling down rapidly.

We will revisit that decision possibly before v7 is cut and definitely before v8 is cut next April. That gives us at least 6-7 months to play with to figure out a better approach.

4½ months ago, when I notified Gulp, around the time when v6 was released, I was informed that it is going to be fixed on the Gulp side in time to Node.js v7 release. Now it seems that it would not. Why do we think that in would be fixed in the _next_ 6 months? And what do we do, if it's not?

A reminder: #6413, #6749, #6573, #7162, and #2025 are being blocked here.

[phated]

@ChALkeR fwiw, it is already fixed in the gulp 4 working branch but that is unable to be published due to other breaking changes that need to land. Please stop derailing this thread with specific discussion about gulp and graceful-fs. I followed up on your linked thread.

[isaacs]

I am not done reading the thread, and will probably not keep up with it, this not being my fulltime job any longer.

But I want to make something very clear. I'm not complaining about "one of my modules broke". I _am_ complaining, and I want you all to understand exactly what I'm complaining about, because it's much more important to me than graceful-fs.

This is not about graceful-fs. This is about Node.js.

I am complaining about a lack of clarity in the articulation of the values of the Node.js project.

Now, either moving things to internal modules is _a mistake_ (in the sense of "Oops, we didn't mean to do that, didn't notice that it violates our goals, let's fix the bug"), or it's a representation of a lived value (but perhaps one that is out of sync with the stated values), or I'm just missing something (which is what I assumed, hence asking for a link to some kind of articulation of how doing this serves Node.js's stated values.)

If your lived values are out of sync with your stated values, then that is a recipe for dysfunction and conflict. (Observe the reactions to almost every graceful-fs bug. "You're abusing an API!" "No, you're abusing it by exposing it!" etc, to wearyingly repetitive infinitum.)

There is a very clear articulation of the Node project's values in the Guiding Principles statement linked in the OP. This articulation of stated values seems to indicate that moving APIs to an internal un-exposed location is a _cost_ rather than a _benefit_. Why _shouldn't_ monkeypatching be allowed? It seems like a pretty obvious thing that should be preserved, according to the guiding principles. (Maybe not according to your personal coding esthetic, and that's fine, because Node.js is a community project, and not anyone's personal hobby, so we all make compromises.)

If you want to say that something makes it worth the cost, ok, fine. What value is being served? Can someone write that down somewhere? I'm assuming I'm just missing it.

If you want to say that, no, actually, we value being able to make changes more than we value avoiding userland disruption (or some other thing, whatever), then that's fine, too. But in that case, can the TSC please ratify that change to the guiding principles, if the linked principles are not the ones _actually_ guiding the project?

What you can't say, according to Node.js's guiding principles, is "We're breaking userland because userland is bad". If that's honestly the answer, then that's unacceptable and either the guiding principles has to change, or the answer does. I'm honestly fine with either.

All that I'm asking for is clarity. No one is abusing anything. Stated values being out of sync with lived values is a normal thing, and there's lots of prior art. It can be solved by either changing a policy or changing stated values, or just writing up a justification for a decision. It's all very straightforward typing problems. Let's all take the fight down a notch. I'm just reporting what looks like a bug, it's fine.

[ChALkeR]

@phated Sorry, I didn't want to derail the discussion, I was mostly answering the concerns that were brought up above, and I mentioned Gulp only in replies to that. No offence meant. =)

[MylesBorins]

@isaacs I think that this conversation has begun with a misunderstanding, which is unfortunate.

At the time that these changes were originally made, it was not exactly obvious that moving things to internal broke anything. It was originally just trying to print a deprecation message. We have been using internal/deprecate for quite some time, it was just by luck it hadn't happened in fs, and by that luck graceful-fs was never broken.

As soon as it was obvious that it was broken the change was reverted.

There are a number of changes lined up to land in fs relying on internal functions, the most important of which is https://github.com/nodejs/node/pull/6573. This PR would enable us to change error messages without making having to do a semver major bump. It will also allow error messages to be internationalized, which imho is pretty rad.

The discussion was brought up to decide, can we do this without breaking. As of Monday we thought we could, so we were ready to do it, it turns out we were mistaken. Today it was decided that we are going to figure out a way forward that does not break the ecosystem.

Personally I don't see our current values at odds with the guiding principals, but perhaps we should take a moment to step back a re-evaluate them. If that is indeed the intention of this thread I think we have unfortunately derailed past that. I'll open an issue on the TSC repo for us to discuss that, not to further thwart progress, but because i think that is the appropriate place to get a definitive answer on the subject.

The main thing that I want to articulate in this post is that at no time was there a decision made about what would or wouldn't be done. Individuals on the CTC have various opinions on the best way to approach this, but in the end we decided to move carefully. Perhaps we need a better way to signal to the community of package maintainers and consumers the way this process works.

I truly believe there is a path forward here if we all work together, our goals are aligned even if it doesn't seem like it. We all want to see node improve, and it will. We are not quite accustom to Semver Major changes, and ecosystem breakages... and as a community we need to find better ways to approach these subjects.

TLDR; We all care too much.

[Fishrock123]

TLDR; We all care too much.

oh my god this

[isaacs]

Thanks for the links and explanation, @TheAlphaNerd

TLDR; We all care too much.

It's not a matter of caring too much or too little, but rather caring effectively and transparently.

6573. This PR would enable us to change error messages without making having to do a semver major bump. It will also allow error messages to be internationalized, which imho is pretty rad.

Is changing error messages a semver major bump today?

It seems like this makes a strong case for moving error message strings into _some_ centralized location, but I'm still kind of O_o? about why it's valuable for this (or anything else in core) to be inaccessible by userland code. It seems like it is contradicted by the value of having core be as small as possible, and empowering userland code as much as possible. Requiring a semver major bump seems like a _positive_ in that case, because it prevents change that doesn't have enough of a community benefit to increment an integer. (A version number is a pretty low bar, after all.)

Again, I'm not saying it's _not_ valuable, just that the value seems unclear to me, in the sense of being tied back to Node's stated values. The issue here is clarity.

I truly believe there is a path forward here if we all work together, our goals are aligned even if it doesn't seem like it. We all want to see node improve, and it will.

I appreciate your positive sentiment and confidence in everyone's good faith. However, without clearly articulated values, I literally don't know what the words "forward", "goals", or "improve" mean in the context of those two sentences, so I can't confidently say whether or not they're "aligned".

I'm really not out to nitpick you here, I know it probably seems like I'm being difficult. I'm just trying to help drive the conversation towards understanding by highlighting the points that are currently sources of confusion.

[jasnell]

Is changing error messages a semver major bump today?

Yes.

[ChALkeR]

@addaleax

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?

[imyller]

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 internal libs) should be supported; maybe even expected.

[ChALkeR]

@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-stream and safe-buffer modules 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 =).

[imyller]

@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 of var 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.

[isaacs]

@Fishrock123 Sorry, I didn't notice your comment in the stream. This is a reply to https://github.com/nodejs/node/issues/8149#issuecomment-240533377

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/internal except 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_.

[AndreasMadsen]

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.defineProperty was 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._

[Trott]

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.

[refack]

Just as a reminder --expose-internals is available for users who want to explicitly opt-in to use internal/ modules, without it being implicitly inherited by required modules.

[Fishrock123]

It is _available_ but that does not mean that we will support anything that uses it.

[targos]

Yeah, it is not meant to be used by users. The flag is here for our own usage in tests and benchmarks.

[jasnell]

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 of internal/... is absolutely unsupported.

[refack]

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-internals is subject to breaking changes. IMHO it could be similar to the stetment about experimental APIs
Ref: https://github.com/jasnell/node/blob/314b78799c5754c494b505cedd815a973c123f03/doc/api/documentation.md#stability-index

[vkurchatkin]

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 of internal/... is absolutely unsupported.

One could actually have internal package inside of their node_modules, so this is perfectly valid

[jasnell]

One 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-internals is used and the require is referencing an actual internal module.