dicer
dicer copied to clipboard
mscdex
lib: use `Writable#_final` for 'finish' tracking
Instead overriding emit method and using this._realFinish to detect
unexpected end of data - implement _final method and track the logic
there.
The only behavior change from the use of _final is that we would no
longer emit finish when the input data is terminated early. Instead we
would emit error as before and stop the processing. Note that this is
the behavior provided by stream.Writable, and it is thus conformant
with the specification.
Additionally, replace uses of private _events property with either
straight emit() with return value check, or listenerCount() in the
situations where there might be an overhead from the constructed event
arguments.
Fix: #26
cc @mscdex (this is @indutny under disguise)
I didn't run the formidable benchmarks, but the ones in the repo appears to be giving roughly the same timing as before the patch:
indutny@Fedors-MacBook-Pro-2 dicer % git co master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
indutny@Fedors-MacBook-Pro-2 dicer % node bench/bench.js dicer
dicer: 419.234ms
indutny@Fedors-MacBook-Pro-2 dicer % node bench/bench.js dicer
dicer: 432.782ms
indutny@Fedors-MacBook-Pro-2 dicer % git co fix/pipeline-incompat
Switched to branch 'fix/pipeline-incompat'
Your branch is up to date with 'sig/fix/pipeline-incompat'.
indutny@Fedors-MacBook-Pro-2 dicer % node bench/bench.js dicer
dicer: 418.913ms
indutny@Fedors-MacBook-Pro-2 dicer % node bench/bench.js dicer
dicer: 423.359ms
FWIW, I've tested it on our codebase (Signal-Desktop) and the pipeline() test pass there as well.
Oof, this was tricky. Node 10 and 12 have different event behavior from 14 and 16. In particular, I get both error and end events on part and dicer on <= 12, and only 'error' on > 12.
Additionally, I had to delay the cb() in _final because it wasn't giving the part a chance to fire its error on Node <= 12.
Weird stuff!
Anyway, it is all fixed now, although I'm much less happy with the PR than I was at the start. (Hacks :sob:)
Totally no rush with this. Just let me know if you'll ever need any additional information or more detailed description of the changes to help you review this. Thanks!
(Friendly weekly reminder so that this PR doesn't fall through the cracks of your inbox)
I ran into this issue today, using pipeline from stream/promises. Wondering why this PR hasn't been merged 😅
