GeezerVison

We have been moving in the opposite direction for many apis... Making them more difficult or impossible to monkey patch for both maintenance and security reasons. I'm not sure this pr is the right approach. We should revisit or strategy around this and build better hook points that are safer and supportable.

@jasnell: I get your point but I think I disagree. This is more about composition than encapsulation. We are allowing the user to pass a public API interface. For me this is no different than letting users create their on read and write streams using Readable and Writable.

e.g. graceful-fs, rather than rolling their own implementation they chose to monkey patch. Which is something we should discourage, but still we need to provide reasonable alternatives.

I'll fix @addaleax comments once there is consensus.

@jasnell I don’t think it qualifies as monkey-patching if we provide a public API for overriding methods. In my opinion this is exactly the kind of approach we should take in order to avoid monkey-patching.

/cc @nodejs/tsc

I’m not convinced that there is so much code in WriteStream or ReadStream that can’t easily be duplicated in a module, if one desired so. I’m unsure if this would solve the graceful-fs problems or not.

Right now graceful-fs creates new ES5 object constructors which create objects that extend fs.ReadStream and fs.WriteStream with an overridden open method to enable retry on ENFILE/EMFILE. Even this little change means that the overridden open method can (does) diverge from the node.js method. The native WrtieStream in node.js 5.5.0+ supports an autoClose: false option to prevent stream.destroy() from being called when fs.open has an error. graceful-fs currently performs an unconditional stream.destroy() upon error from fs.open in WriteStream.prototype.open. I think completely re-implementing the fs streams code in graceful-fs would lead to more chances of undocumented differences from native fs. Obviously graceful-fs is meant to be different from native fs, but only in the documented ways.

This change would allow graceful-fs to skip re-implementing the stream open methods if run under node.js 13+, it would be able to just inject the fs option to the constructor instead.

As opposed to overriding like this or monkeypatching, what I'd prefer to see is the improved ability for developers to create their own alternative implementations using supported lower level APIs... Something more along the lines of...

class MyAlternativeWriter extends fs.FileWriteBase {
  open() { /* ... */ }
  close() { /* ... */ }
  /* ... */
}

While the differences in the approaches may be subtle, this would allow anyone inspecting the object and seeing the MyAlternativeWriter object name to know that they are working with something that operates differently from fs.FileWriter.

Ftr, I liked the previous approach (passing an fs object), but it doesn’t make a huge difference anyway.

I'm fine with either...

@jasnell I don't think this is monkey-patching, it is introducing an API so that monkey-patching wouldn't be used. That API would be documented and its usage would be visible. Also, we could just forbid this call with policies or harden method later, security-wise.

In general, I am in favour (if this would help to minimize monkey-patching), but I am not sure about the API yet and I don't think that open/read/close is enough here? But that's not a blocker.

Ftr, I liked the previous approach (passing an fs object), but it doesn’t make a huge difference anyway.

I do agree with @jasnell that having the stream internally store references to the individual methods is best so the stream ignores any monkey-patching done after construction but I think it would be simpler for the public API to just accept a single fs option. I'm not stuck on this one way or the other just sharing my preference.

With the fs option you could still test with the following:

fs.createReadStream(fn, {
  fs: {
    open: common.mustCall(fs.open),
    read: common.mustCallAtLeast(fs.read, 1),
    close: common.mustCall(fs.close)
  }
});

rebased, @coreyfarrell: another CI?

CI: https://ci.nodejs.org/job/node-test-pull-request/27579/

@Trott: This looks ready to land?

CI: https://ci.nodejs.org/job/node-test-pull-request/27601/

Sorry, I made a git mistake with the last one. Can we start another CI?

CI: https://ci.nodejs.org/job/node-test-pull-request/27610/

@ronag Linter is still failing…

How do I run the linter without having to rebuild the whole thing?

How do I run the linter without having to rebuild the whole thing?

If it's just JavaScript linting, make lint-js (or vcbuild lint-js on Windows) shouldn't require a full rebuild, I don't think.

CI: https://ci.nodejs.org/job/node-test-pull-request/27700/

(Whoever lands this should edit the commit first line for typos since those end up displayed prominently in the change log files.)

CI: https://ci.nodejs.org/job/node-test-pull-request/27776/

Landed in 2b9847c.