refactor: use stream.finished() instead of custom implementation #56
Conversation
| if (isFinished(msg) !== false) { | ||
| defer(listener, null, msg) | ||
| setImmediate(listener, null, msg) | ||
| return msg | ||
| } |
There was a problem hiding this comment.
The if block can be removed if we only accept Streams. There is a test with an unknown object blocking this change:
it('should invoke listener given an unknown object', function (done) {
onFinished({}, done)
})
|
I'm in favor of this if it ensures that body-parser keeps working with AsyncLocalStorage More than the tests passing here, I mean that they should also pass over there I hope to finish the WIBY prototype soon, to make this easier and to ensure we don't break anything. |
|
@bjohansebas I opened PRs in all the packages that use
So far everything is passing. |
There was a problem hiding this comment.
Spent a day or so reviewing, this looks solid. marking changes requested for the socket listener cleanup when msg completes first.
I considered suggesting we reuse stream.finished on the socket also, but really we only care about the close, error events there so this is fine. Socket is duplex, so we'd get the duplex handling from stream.finished unless we configured it away depending on the direction (req/res).
For anyone interested in what stream.finished is doing:
https://github.com/nodejs/node/blob/v22.15.0/lib/internal/streams/end-of-stream.js#L54
index.js
Outdated
| const asyncHooks = require('node:async_hooks') | ||
| const stream = require('node:stream') |
There was a problem hiding this comment.
| const asyncHooks = require('node:async_hooks') | |
| const stream = require('node:stream') | |
| const asyncHooks = require('async_hooks') | |
| const stream = require('stream') |
Depending on the version support range we want to land on here, node:foo didn't go stable available until node 16 I think?
There was a problem hiding this comment.
@jonchurch What version range do we want to support here? Since this change requires a new major release anyway, we could take the opportunity to bump the range to ">=18" to match what Express is using.
There was a problem hiding this comment.
I don't have a principled opinion here right now. The project as a whole may not either.
18 would be pragmatic, but we can get away with 10.
For context: the main reason this package is still useful IMO is that it combines the monitoring of sockets along with streams to determine when a req/res has "finished". I believe there are still some edgecases where a socket can close and the req/res would miss the close event. Hence the need to solve in libraryland vs relying solely on the platform (aka stream.finished doesn't introspect to check if the input has a socket property).
Because the change here is minor (nobody really needs this feature alone) targetting 18 has low risk of leaving someone behind.
Phillip9587
force-pushed
the
stream-finished
branch
from
4fe2397 to
b764cbd
Compare
|
@jonchurch I added the socket listener cleanup. Could you please take another look? |
Context
There have been previous efforts to modernize usage of
on-finished, particularly around replacing it withstream.finished(), while still depending onisFinished()method fromon-finished.This PR tries to consolidate and properly implement those ideas within
on-finisheditself.Related PRs:
on-finishedwith nativestream.finishedfor request handling expressjs/body-parser#598Changes
This pull request refactors the
on-finishedpackage to use the nativestream.finished()while maintaining compatibility with previous behaviour.ee-firstlibrary with thestream.finishedutility from Node.js for handling message and socket events, improving compatibility with modern Node.js versions. (index.js, index.jsL92-R137)patchAssignSocketandtryRequireAsyncHooksfunctions, as they are no longer relevant. (index.js, index.jsL179-R182)BREAKING CHANGES
stream.finished()is available since Node.js 10.0.0.cc: @bjohansebas @UlisesGascon