-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: simulate body events #7
Conversation
@@ -30,6 +31,16 @@ export function createCall (handle: Handle) { | |||
// @ts-ignore | |||
req.body = context.body || null | |||
|
|||
if (context.body) { | |||
// @ts-ignore | |||
process.nextTick(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
process.nextTick
is important here, this is necessary to postpone emit call and make sure listeners are set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work in a non-Node environment, even with the unenv process polyfill. Is there another way to implement this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think createCall
is deeply depends on node environment, because IncomingMessage
extends Readable
and it extends Node's EventEmitter import { EventEmitter } from 'events'
As for process.nextTick
, we need a way to ensure listeners are set, as a solution we can implement EventEmitter logic and create a delayed emit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably use defer polyfill but seems actual readable implementation waits for first listener to drain data: https://github.com/nodejs/node/blob/master/lib/internal/streams/readable.js#L311
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use newListener event too or spy on
prototype method (https://github.com/nodejs/node/blob/602fe4e83a572359c08c3a28fe054addb4252153/lib/internal/streams/readable.js#L902)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danielroe While not related, i'm curious why polyfill not working? It seems has polyfill for nextTick
too (https://github.com/defunctzombie/node-process/blob/77caa43cdaee4ea710aa14d11cea1705293c0ef3/browser.js#L134)
(closing since timeout is not a good solution. since having workaround, let's redo it later and track in linked issue) |
This PR create a simple simulation for body streaming based on https://github.com/unjs/h3/issues/29#issuecomment-891951452
resolves unjs/node-mock-http#8