Revert async formatters #1377
Revert async formatters #1377
Conversation
|
Unit tests need to be updated to validate the new behaviour, but would like a review first in case there are any major changes 😄 |
|
On the surface this looks good but I would like to see the unit tests as unit tests help me with understanding the code better. |
hetul
left a comment
hetul
left a comment
There was a problem hiding this comment.
I think the approach looks good. The refactor around handling the optional __send arguments is a definite improvement!
| * @param {Object | Buffer | Error} body the content to send | ||
| * @param {Object} headers any add'l headers to set | ||
| * @param {Function} callback a callback for use with async formatters | ||
| * @returns {Object | undefined} when async formatter in use, returns null, |
There was a problem hiding this comment.
You may have already planned to do this, but consider updating the response.md documentation to be consistent with the new JSDoc in this PR, just so you don't forget to do it later.
There was a problem hiding this comment.
Good catch! I hadn't looked at documentation, would have missed this ❤️
* Matches definition of __send
No longer needed since the feature has been removed
|
Tests caught a bug where Removed all async formatter specific tests. Only tests failing now are node 8 specific. Ready for another review, assuming everyone is 👍 on this it's ready for a merge ^_^ |
|
I suppose fixing node 8 support is on another ticket? In any case, LGTM! 👍 |
rajatkumar
left a comment
rajatkumar
left a comment
There was a problem hiding this comment.
Minor code comment needed, rest looks good!
| // Populate our response object with the derived arguments | ||
| self.statusCode = code; | ||
| self._body = body; | ||
| Object.keys(headers).forEach(function (k) { |
There was a problem hiding this comment.
Good to see the headers are being set here instead of _flush()
| type = mime.lookup(type); | ||
| } | ||
|
|
||
| if (!self.formatters[type] && self.acceptable.indexOf(type) === -1) { |
There was a problem hiding this comment.
Can we add a code comment explaining why we set type to application/octet-stream
|
Only tests failing on CI are v8 related. Squashing and merging 🎉 |
Pre-Submission Checklist
make prepushIssues
Closes:
Async formatters were introduced as a way to handle formatting an response body by binding it to data coming from an asyncronous data source. This introduced complexities that were difficult to reconsile with the rest of the code base. By requiring all formatters to be syncronous we simplify the codebase and can 👏 ship 5.x 👏
Changes
__sendimplementation