search

mafintosh / duplexify

Turn a writable and readable stream into a streams2 duplex stream with support for async initialization and streams1/streams2 input
MIT License
190 stars 36 forks source link

Updated to readable-stream@3 #25

Closed mcollina closed 5 years ago

mcollina commented 5 years ago

As titled.

This is semver-major.

It would be good if it included https://github.com/maxogden/concat-stream/pull/61, but that's only a dev dependency, so it can land otherwise.

vweevers commented 5 years ago

The handling of (_)destroy, destroyed and _finish vs _final should be updated.

For destroy, it might suffice to simply remove the following:

https://github.com/mafintosh/duplexify/blob/683ee893bb34202d1f00228e184ca38dfb849d50/index.js#L57

https://github.com/mafintosh/duplexify/blob/683ee893bb34202d1f00228e184ca38dfb849d50/index.js#L176-L184

https://github.com/mafintosh/duplexify/blob/683ee893bb34202d1f00228e184ca38dfb849d50/index.js#L199

mcollina commented 5 years ago

@vweevers I've updated destroy, but I don't know what you mean with _final vs _finish change.

vweevers commented 5 years ago

I don't know what you mean with _final vs _finish change

I think the _finish method here is custom logic for delaying the finish event, using the undocumented prefinish event and _writableState.prefinished. We now have _final for this use case.

mcollina commented 5 years ago

@vweevers done!

vweevers commented 5 years ago

Looks OK to me! Can't say I fully understand the internals though. What's the purpose of ondrain, and why is it called in _destroy?

https://github.com/mafintosh/duplexify/blob/09f469620d31c97153f3f5194154f755294854bd/index.js#L174-L179

mcollina commented 5 years ago

ondrain is a user specified function.

vweevers commented 5 years ago

ondrain is a user specified function.

There's this too:

https://github.com/mafintosh/duplexify/blob/09f469620d31c97153f3f5194154f755294854bd/index.js#L194

mcollina commented 5 years ago

I’ll take a look, it might not be needed after all.

mcollina commented 5 years ago

ondrain was not really needed in _destroy(), it' all handled by _destroy now.

hxtomprice commented 5 years ago

@mafintosh did you mean to bump the version of readable-stream in master without these changes here?

We're seeing stack traces with the latest duplexify release which is required by @google-cloud/pubsub

Error: Cannot call write after a stream was destroyed
    at doWrite (/src/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:405:38)
    at writeOrBuffer (/src/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:394:5)
    at Duplexify.Writable.write (/src/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:303:11)
    at Duplexify.end (/src/node_modules/duplexify/index.js:230:41)
    at Immediate.onConnectionStatus (/src/node_modules/@google-cloud/pubsub/src/connection-pool.js:275:18)
    at runCallback (timers.js:706:11)
    at tryOnImmediate (timers.js:676:5)
    at processImmediate (timers.js:658:5)
    at process.topLevelDomainCallback (domain.js:121:23)
mafintosh commented 5 years ago

@hxtomprice hmm, let me take a look

mafintosh commented 5 years ago

@hxtomprice master should work fine, i can't see where dupelxify is used in that repo. you have a link?

mafintosh commented 5 years ago

Gonna revert the one in master and move it to a new major instead to be safe. My guess is that it's a timing change as the destroy semantics are more rigourous now.

vweevers commented 5 years ago

@mafintosh Do you have the intention of merging this later on? Maybe it wouldn't make a functional difference, but it does serve an educational purpose. Personally I learned more about streams from reading the source code of modules like this, than from reading streams core (with its complicated state). I think it'd benefit the community if modules use patterns like _destroy and _final as much as possible, to advocate best practices, advertise the abilities of streams, make code easier to understand for beginners and ultimately lead to a more homogeneous ecosystem. Thoughts?