Avoid setting Content-Type on 304

zbarbuto commented 6 years ago

Description

Fixes a bug where loopback tries to set headers that have already been sent.

Note that express will strip out the Content-Type header if the response is 304:

https://github.com/expressjs/express/blob/351396f971280ab79faddcf9782ea50f4e88358d/lib/response.js#L208-L214

Loopback will try to set Content-Type to a default (typically application/json) if the header does not exist

https://github.com/strongloop/strong-remoting/blob/2153d183be457e8d8f76ca08b79fedb87b387b15/lib/http-context.js#L573-L575

This can lead to a weird edge case when doing tasks like rendering ejs where the following occurs:

Application:

Tell res to render a template (e.g. an ejs page) from a remote method

Express

Set some headers including Content-Type: text/html since we’re rendering ejs
Realise that the statusCode is 304 so strip out some irrelevant headers including Content-Type

Loopback:

Check to see if we have a Content-Type set (which we now don’t because 304 stripped it out)
Try to set default Content-Type to application/json

Express:

Throws error as headers have already been send (unhandled promise rejection)

Related issues

Nil

Checklist

[x] New tests added or existing tests modified to cover all changes
[x] Code conforms with the style guide

zbarbuto commented 6 years ago

Please advise of failures as CI is closed to the public 😞

bajtos commented 6 years ago

Please advise of failures as CI is closed to the public

Please take a look at Travis CI, that on is publicly visible.

 1) strong-remoting-jsonrpc handlers jsonrpc should support calling object methods:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

The lack of visibility into cis-jenkins build failures is a know pain point :(

zbarbuto commented 6 years ago

@bajtos Thanks for feedback. Have switched to promise-based test and switched from var to const.

bajtos commented 6 years ago

[cis-jenkins] x64 && linux && nvm,4 — Failed! (34971e5)

The failure is unrelated to the changes made in this pull request, our CI was not able to fetch some dependencies.

bajtos commented 6 years ago

Landed, thank you @zbarbuto for the contribution!

strongloop / strong-remoting