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

setHeader method may be set incorrectly #122

Open
MikeMangum opened this issue Feb 14, 2020 · 7 comments
Open

setHeader method may be set incorrectly #122

MikeMangum opened this issue Feb 14, 2020 · 7 comments

Comments

@MikeMangum
Copy link

The method used to set the header won't work for mocked requests:

var setHeader = res.set ? http.OutgoingMessage.prototype.setHeader : res.setHeader

Should probably be something along the lines of:

var setHeader = res.setHeader ? res.setHeader : http.OutgoingMessage.prototype.setHeader

@dougwilson
Copy link
Contributor

The issue is that if it is reversed it will break what the workaround is for. There are some frameworks with a broken / different res.set , so this uses the original instead when it is there. But they have a (broken in the same way) res.setHeader as well.

@dougwilson
Copy link
Contributor

This is the most recent comment that made the current implementation for reference: e3b1247

I will see if I can dig up the original issue / PR as well when I get back to a computer.

@dougwilson
Copy link
Contributor

PR: #19

@MikeMangum
Copy link
Author

My concern is that this is a workaround for a problem with a specific implementation of setHeader function but it is not targeted to that specific implementation.
var setHeader = !(res.__proto__.app === undefined) ? http.OutgoingMessage.prototype.setHeader : res.setHeader

This would use the setHeader function of the passed in ServerResponse instance except in the specific case that it is express'
implementation, which (if I understood the issue correctly) is the issue fixed in that PR.

@dougwilson
Copy link
Contributor

I agree it is bad and I'm not sure i would have landed that pr if it were i, but it has been landed and is now part of the api. If you are ok with waiting a bit for me to get together a major release we can just remove it, otherwise just need a change that will not break folks .

@MikeMangum
Copy link
Author

Sounds good.

@mrfitz42
Copy link

mrfitz42 commented Apr 9, 2024

How goes the progress on this? I'm working on unit tests for a system we are upgrading from node 4.4.7 and have found that this is causing an exception when using node-mocks-http to create res.

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

No branches or pull requests

4 participants