Add checkOpen option

[ehmicky]

Fixes #7.

The methods in this package check two different things:

1. Whether the argument has the correct stream type
2. Whether the stream is writable/readable, i.e. whether it has been closed

There are times when one would want to check only 1. but not 2.. For example:

• The user might not need the stream to be open since many stream core methods (finished(), .pipe(), etc.) work fine with closed streams.
• The user might want to provide with a different error message for 1. and 2.

This PR adds a checkOpen boolean option to toggle off that specific check.

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

[ehmicky]

stream.writable and stream.readable are always booleans.

https://github.com/nodejs/node/blob/ec3040f7210988440ab5a31519c8637599f3bb17/lib/internal/streams/readable.js#L1413

https://github.com/nodejs/node/blob/ec3040f7210988440ab5a31519c8637599f3bb17/lib/internal/streams/writable.js#L1001

[sindresorhus]

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?

[ehmicky]

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.

is-stream/package.json

Line 16 in 45e90c2

"node": "^12.20.0 || ^14.13.1 || >=16.0.0"

[sindresorhus]

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 👍

[ehmicky]

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.

[ehmicky]

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

[ehmicky]

I implemented the solution above: ready for review 👍