By default, it is false for isStream() and true for the others to keep backward compatibility. I.e. this is not a breaking change.

Is this a default you would have chosen if you made this package from scratch today?

Probably not. The better behavior would probably be for checkOpen to always default to true, for all methods including isStream().

I wanted to avoid any breaking change, but I was also suggesting making a major release with #21, for safety, so I could change the default value if you want?

If you'd want to do a major release, I can also do a PR to update the supported Node.js range.

I wanted to avoid any breaking change, but I was also suggesting making a major release with #21, for safety, so I could change the default value if you want?

Yeah, then lets change the default.

If you'd want to do a major release, I can also do a PR to update the supported Node.js range.

I can handle that when releasing 👍

There is one catch though. The checkOpen option uses the readable and writable properties. Those properties are always defined (I checked the source code) for all stream classes exposed by core Node.js:

• stream: Readable, Writable, Duplex, Transform, PassThrough
• fs: createReadStream(), createWriteStream()
• http: OutgoingMessage, IncomingMessage, ServerResponse, ClientRequest
• net: Socket

However, those inherit all from a base class stream.Stream. That base class is actually quite useless:

• It is undocumented (although it can be imported and used)
• It is called legacy.js in the source code
• It only does two things:
  • Inherit from EventEmitter
  • Add a .pipe() method, which is overridden by most of the classes above

Unfortunately, that base class does not define any readable/writable property. But its .pipe() method does expect those properties to exist.

https://github.com/nodejs/node/blob/0550bc149c4e9e13e861f48a112fedde360616db/lib/internal/streams/legacy.js#L20

https://github.com/nodejs/node/blob/0550bc149c4e9e13e861f48a112fedde360616db/lib/internal/streams/legacy.js#L28

So technically, one could inherit from this undocumented base class and miss those properties. But then the .pipe() method would not properly work. And it would differ from all other stream classes.

I thought of a solution to support the above edge case: if stream.writable and stream.readable are both undefined, this means an instance of the Stream base class was passed as argument. If that happens, checkOpen should be a noop with isStream() (but not with other methods).

I implemented the solution above: ready for review 👍