fix: refactor to node streams #14
Conversation
ronag
force-pushed
the
refactor-stream
branch
2 times, most recently
from
d23b9c4
to
053be16
Compare
ronag
force-pushed
the
refactor-stream
branch
2 times, most recently
from
25ac443
to
ade2dec
Compare
ronag
force-pushed
the
refactor-stream
branch
from
ade2dec
to
34999e9
Compare
|
Hi @ronag, Thanks so much for opening this and addressing the compatibility issues with node 13. I've skimmed your changes and am very interested in this approach. I will give this a deep dive tomorrow. |
| @@ -2,91 +2,62 @@ import crypto from "crypto"; | |||
| import fs from "fs"; | |||
| import os from "os"; | |||
| import path from "path"; | |||
| import { Readable, Writable } from "readable-stream"; | |||
There was a problem hiding this comment.
readable-stream is needed for Node 8 compat
|
|
||
| this.bytesWritten = 0; | ||
| this.pos = 0; | ||
| this.closed = false; |
There was a problem hiding this comment.
closed and bytesWritten are not really needed but I'm not sure what the expected public API here is and they remain to not unnecessarily further break compat. But I would recommend deprecation and removal.
| if (!error) this.emit("write"); | ||
| callback(error); | ||
| }); | ||
| get finished() { |
There was a problem hiding this comment.
This is also not really needed and stream in Node 13+ has a writableFinished property. Remains to not unnecessarily further break compat but I would recommend deprecation and removal. The readable side could just as easily access the internal property.
|
|
||
| this._readStreams = new Set(); | ||
| this.error = null; |
There was a problem hiding this comment.
Removed error prop as it's a bit strange. Though I'm unsure how important it is for the public API and is a semver-major breaking change that needs to be considered.
| this.once("open", this._destroy.bind(this, error, callback)); | ||
| return; | ||
| } | ||
| if (this.destroyed) return; |
There was a problem hiding this comment.
This won't be required once readable-stream is updated to Node 13 impl. However, before that Readable does not check for destruction before calling _read and this condition is therefore required.
| this.error = error; | ||
| }); | ||
|
|
||
| this.open(); | ||
| } | ||
|
|
||
| get ended() { |
There was a problem hiding this comment.
This is also not really needed and stream in Node 13+ has a readableEnded property. Remains to not unnecessarily further break compat but I would recommend deprecation and removal. The readable side could just as easily access the internal property.
Refactors module into node streams. There are some rules that should be followed to maintain compatibility with node streams:
destroy()open()(it's deprecated)This fixes breakage in Node 13 nodejs/node#30110.
This is potentially breaking.
Fixes a bug where the capacitor could be destroyed before all readables have been destroyed.
Re-use same fd between write and read.
Slightly slower read since it doesn't re-use the pooling logic from
ReadStream.