Skip to content
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

fix(cloneIncomingMessage): inherit prototype properties #214

Merged
merged 5 commits into from
Mar 1, 2022

Conversation

hrsh7th
Copy link
Contributor

@hrsh7th hrsh7th commented Feb 26, 2022

Fix #212

The current implementation will clone the properties defined in instance.
But the headers property defined in prototype in Node.js v16 or higher.

So this PR aims to support prototype inheriting in cloneIncomingMessage.

The final pitfalls is this is not support instanceof IncomingMessage and instanceof PassThrough. If the msw uses those operators, I think this must not be merged.

Fixed.

@kettanaito
Copy link
Member

Hey, @hrsh7th! Thank you for submitting this fix.

The final pitfalls is this is not support instanceof IncomingMessage and instanceof PassThrough

This may be an issue, as we're exposing the cloned IncomingMessage publicly:

return super.emit(event, firstClone, ...data.slice(1))

When the consumer adds a response listener on ClientRequest, they will get the cloned message when using this library. They may have instanceof and other logic around that, which will break with this change if I understand correctly.

Maybe we can think of reusing parts of your solution to preserve the prototype? Would that be possible?

@trevoro
Copy link

trevoro commented Feb 27, 2022

I've tested this locally on node16 re-using the same workflow that the github actions has and it works great!

Regarding the types, this function as it existed before and after this PR forces the type by casting to unknown and then to ClonedIncomingMessage. The stream itself has the same overall shape but it now has the prototype methods added in. If you remove the 'to unknown' part tsc has a way better idea about the type of the stream and it seems like it's been the wrong one for quite some time. Like if you look at the shape of an IncomingMessage vs what's there in main it's not the same at all. I think that's out of scope of this issue for now though.

@hrsh7th
Copy link
Contributor Author

hrsh7th commented Feb 27, 2022

I'd changed the implementation to support stream instanceof IncomingMessage.

This approach can be broken if both IncomingMessage and PassThrough have the same properties in different implementations. (This fact is the same in the existing implementation. So I think we have not to care about it in this PR for now)

@hrsh7th
Copy link
Contributor Author

hrsh7th commented Feb 28, 2022

I think this PR is ready to review and merge. 👍🏼

expect(clone).toBeInstanceOf(IncomingMessage)
expect(clone).toBeInstanceOf(EventEmitter)
expect(clone).toBeInstanceOf(Stream)
expect(clone).toBeInstanceOf(Readable)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding assertions to check that the cloned message indeed extends all the prototypes of the original message.

@kettanaito kettanaito changed the title fix: improve cloning codes fix(cloneIncomingMessage): inherit prototype properties Mar 1, 2022
Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely fantastic job on this one, @hrsh7th! 🎉
Thank you for making this library better. Welcome to contributors!

@kettanaito kettanaito merged commit 7d8f6bd into mswjs:main Mar 1, 2022
@github-actions
Copy link

github-actions bot commented Mar 1, 2022

🎉 This PR is included in version 0.13.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@hrsh7th
Copy link
Contributor Author

hrsh7th commented Mar 1, 2022

Thank you for review, fix and merge! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cloneIncomingMessage does not handle headers field in Node.js v16
3 participants