Fix "Cannot set property closed of" on Node 18

[dsokal]

Fixes #828

I'm not an expert on Node internals but from briefly reading the source code, it seems the problem is that .closed doesn't have a setter since Node 18 (https://github.com/nodejs/node/blob/v18.0.0/lib/internal/streams/readable.js#L1243).

The fix is to check whether this._readableState.closed / this._writableState.closed exists and use it instead of this.closed. This mimics other setters from the Readable/Writable class.

I also upgraded the node image in CI - to use Node 18.

The test plan is I guess just to see if CI checks pass. I saw #829 and it seems the tests failed there. From my understanding, if my fix is correct, CI checks should pass now. I also ran yarn test locally on Node 18.1 and it succeeded.

[G-Rath]

Suggested change

	if (this._readableState && typeof this._readableState.closed === 'boolean') {
	if (typeof this._readableState?.closed === 'boolean') {

It should be possible to use optional chaining here right?

[dsokal]

@G-Rath I addressed your feedback and also realized that I should fix the FsWriteStream implementation. I decided to copy-paste the close method from FsReadStream because in my opinion sth like the following would look weird:

  if (typeof this._readableState?.closed === 'boolean') {
    this._readableState.closed = true;
  } else if (typeof this._writableState?.closed === 'boolean') {
    this._writableState.closed = true;
  } else {
    this.closed = true;
  }

Especially because _writableState does not exist for ReadStream.

But I can change it, whatever you prefer.

[dsokal]

@G-Rath just a friendly reminder about this PR.

[ianlavapiez]

^^ for this merge request.

[streamich]

🎉 This PR is included in version 3.4.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[pratik136]

I'm inclined to believe that the library should be simultaneously tested on all LTS versions of Node.js in addition to at least the major versions of the recent ones.

Theoretically they all are in active dev, or maintenance, and as such bear the potential to break. If the e2e test suite could be run on all versions, that can act as the first line of defence. Thoughts?

[dsokal]

I agree. I simply changed this value because I'm not a maintainer/code-owner of this lib and this was the easiest I could do to make sure it works on Node 18.