Skip to content

fix(cloneIncomingMessage): inherit prototype properties#214

Merged
kettanaito merged 5 commits intomswjs:mainfrom
hrsh7th:improve-clone
Mar 1, 2022
Merged

fix(cloneIncomingMessage): inherit prototype properties#214
kettanaito merged 5 commits intomswjs:mainfrom
hrsh7th:improve-clone

Conversation

@hrsh7th
Copy link
Copy Markdown
Contributor

@hrsh7th hrsh7th commented Feb 26, 2022 •
edited
Loading

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
Copy Markdown
Member

kettanaito commented Feb 27, 2022

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:

interceptors/src/interceptors/ClientRequest/NodeClientRequest.ts

Line 238 in 4563ce7

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
Copy Markdown

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
Copy Markdown
Contributor Author

hrsh7th commented Feb 27, 2022 •
edited
Loading

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
Copy Markdown
Contributor Author

hrsh7th commented Feb 28, 2022

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

kettanaito
kettanaito reviewed Mar 1, 2022
View reviewed changes
src/interceptors/ClientRequest/utils/cloneIncomingMessage.test.ts
expect(clone).toBeInstanceOf(IncomingMessage)
expect(clone).toBeInstanceOf(EventEmitter)
expect(clone).toBeInstanceOf(Stream)
expect(clone).toBeInstanceOf(Readable)
Copy link
Copy Markdown
Member

@kettanaito kettanaito Mar 1, 2022

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
kettanaito
kettanaito approved these changes Mar 1, 2022
View reviewed changes
Copy link
Copy Markdown
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
@kettanaito kettanaito mentioned this pull request Mar 1, 2022
Merged
@github-actions
Copy link
Copy Markdown

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
Copy Markdown
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

Reviewers

@kettanaito kettanaito kettanaito approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cloneIncomingMessage does not handle headers field in Node.js v16

3 participants

@hrsh7th @kettanaito @trevoro