RFE: Articulate benefits of moving code to internal/... #8149
Comments
addaleax
added
discuss
node version 7 officially deprecates `fs.read` and `fs.readSync` This is done using `internal/util`. The unfortunate side affect being that `graceful-fs v3.x` explodes when running the code in `vm` as `internal/util` is not accessible from userland. This commit uses a regular expression to replaces the require of the specific internal util function with the source of that util function. As such `graceful-fs v3.x` will no longer explode in node v7. One advantage to this approach is that any future deprecation will not break graceful-fs.
|
/cc @nodejs/ctc, @thealphanerd, @thefourtheye, and, perhaps, @phated |
|
Internal modules are supposed to prevent core stagnation because of userland modules (such as |
|
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. |
|
@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 |
|
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. |
|
"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 One great example of this is the The 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) |
I think this is a better description:
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. |
|
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.
I don't see how the internal modules are covered in this, but this does sound like something of an argument against the WIP
Maintainability. If you would like it explicitly stated on the website, I'm sure that would be possible...
I don't understand how this would be the case. Care to elaborate @isaacs? |
«Fault» does not mean anything here, it is not important. According to the above, we had an issue:
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. |
|
@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? |
And we unbroke it, so now we're discussing how to move forward. |
|
@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. |
@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. |
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. |
|
@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 😖 |
|
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.) |
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? |
|
@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. |
|
@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 I believe @thealphanerd did some tests, and all of those passes on |
|
We could just solve all issues here if |
|
@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. |
|
@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). |
That's not a breaking change in the JS API, it's module reevaluation. Imagine if someone was searching for the The API wasn't broken or even affected by anything of this. |
|
@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. |
And that's why we have internal modules now. But still, this has nothing to do with |
|
@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 |
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? |
|
@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. |
|
@bnoordhuis For the record, I proposed a way to fix this on the gulp side in May: gulpjs/gulp#1640 — and I still fail to see how that change could possibly affect anyone, except for the unlikely situation that I described above: #8149 (comment). |
|
@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
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. |
|
@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. |
|
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. |
|
@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. =) |
|
@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 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 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. |
oh my god this |
|
Thanks for the links and explanation, @thealphanerd
It's not a matter of caring too much or too little, but rather caring effectively and transparently.
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 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. |
Yes. |
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 |
|
@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 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 =). |
|
@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 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)
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.
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
Moving code into 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 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. 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 |
|
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. |
|
I've actually been entertaining the idea of emitting a process warning when |
It's there, and it works, so we should explicitly state that use of |
One could actually have |
My apologies, I wasn't clear... the warning would be emitted only if |
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)
The text was updated successfully, but these errors were encountered: