Cleanup on headers handling #370
Conversation
|
|
||
| if (req.method === 'GET') { | ||
| delete result.response.headers; | ||
| } |
There was a problem hiding this comment.
This is not actually needed as component.js is used only by the GET endpoint. Also delete can be not great for performances so I revert the strategy (to not set it in the response and then to make it part of the POST individual result)
| @@ -167,7 +167,7 @@ describe('registry', function(){ | |||
| it('should return the component with custom headers', function() { | |||
| expect(result[0].response.version).to.equal('1.0.0'); | |||
| expect(result[0].response.name).to.equal('hello-world-custom-headers'); | |||
| expect(result[0].response.headers).to.be.deep.equal({'cache-control': 'public max-age=3600', 'test-header': 'Test-Value'}); | |||
| expect(result[0].headers).to.be.deep.equal({'cache-control': 'public max-age=3600', 'test-header': 'Test-Value'}); | |||
There was a problem hiding this comment.
I actually think I didn't pay so much attention on the previous review as I was surprised to see this. What do you think @ivan-dimitrov-skyscanner ? Can we safely change this and consider it as a bugfix?
There was a problem hiding this comment.
@matteofigus It seems as a kind of invariant to me. However I tried to checkout your changes locally and play it them and for some reason I could not see how to do it :-(
And the main thing we'd liked to accomplish was:
- in case of GET the headers should be visible as normal HTTP response headers
- in case of POST they should not be set as HTTP response headers (because there is no point to do it) but to be present in the normal response body.
And from this point of view your change looks OK to me.
There was a problem hiding this comment.
Yeah agree. I think the purposed change here is to move headers one level down on the POST so that instead of having
{
components: [{
status: 200,
response: {
...
headers: { ... }
}
}]
}we have
{
components: [{
status: 200,
response: {
...
},
headers: { ... }
}]
}There was a problem hiding this comment.
Agree, and also I'm OK to have it as a bugfix instead of a breaking change :-)
| @@ -453,8 +453,7 @@ describe('registry : routes : component', function(){ | |||
| }); | |||
|
|
|||
| it('should set response headers', function() { | |||
| expect(response.headers).to.not.be.null; | |||
| expect(response.headers['test-header']).to.equal('test-value'); | |||
| expect(response.headers).to.be.undefined; | |||
There was a problem hiding this comment.
I think this tests were actually broken. The GET shouldn't have headers inside the response so this fixes it.
There was a problem hiding this comment.
Hey @matteofigus, thanks for getting back. Can you give me one more day to revisit it as well.
There was a problem hiding this comment.
No problem take your time ;)
There was a problem hiding this comment.
You're right - those tests were broken :-(
|
@matteofigus, |
|
Thanks, so let me merge this and then we'll review that one ;) |
While cleaning up a bit the code (I wanted to remove the
deleteas it may be not great for v8's optimisation, I had the chance to look again at all the tests and I found a couple of things that I'd like to change.response.headersto justheaders(one level down). I think this allows us to have "response" to be exactly identical as a GET /component response.I would be tempted to publish this as a bugfix instead of breaking change if you agree @ivan-dimitrov-skyscanner
Also, we may need to revisit your PR #347 as I think if we have all the headers in the POST perhaps we don't need that?
More details inline.