feat: Add mung.write function
#14
Conversation
index.js
Outdated
| // If response type is not application/json, | ||
| // just call the original res.write function | ||
| if (!(contentType && ~contentType.indexOf('application/json'))) { | ||
| return original.call(res, body); |
index.js
Outdated
| } | ||
|
|
||
| // If null, then 204 No Content | ||
| if (!modifiedJSON) { |
There was a problem hiding this comment.
mung;.json allows the result object to be modified in-place or a new object returned. This only allows a new object to be returned by the mung function.
index.js
Outdated
| return res.status(204).end(); | ||
| } | ||
|
|
||
| res.set('Content-Length', Buffer.byteLength(JSON.stringify(modifiedJSON), 'utf8')); |
There was a problem hiding this comment.
What about cases when charset other than utf8 is used?
There was a problem hiding this comment.
Good catch. I can add encoding as an option.
…ion, call -> apply fix on original calls
|
@hedgeday thanks for all your hard work! Could you also modify the readme to describe this new function. Also, please update the contributors section of |
|
@richardschneider Happy to contribute! 👍 I was thinking about the use case overall, and I'm starting to think it would make sense to have a more generic Also for the record it looks like your comments got buried after I patched that commit with minor formatting changes (my bad), so I'm posting it here again. I'll update the encoding value to use the node res.write default (which, as per the docs, is 'utf-8') and add tests. I'll update this PR to reflect those changes. |
test/write.js
Outdated
| }) | ||
|
|
||
| function appendText (chunk, req, res) { | ||
| return (chunk + ' with more content') |
There was a problem hiding this comment.
I know you mentioned that mung.json has the ability to modify the passed-in object in place, but in the case of res.write, since we are dealing with immutable strings and/or buffers, the function will have to return the manipulated value. I can add this constraint to the README along with the description of this function.
index.js
Outdated
| } | ||
|
|
||
| try { | ||
| let modifiedChunk = fn(chunk, req, res); |
There was a problem hiding this comment.
Let's pass the encoding to the mung function. Make sure null is passed if its really the callback.
Something like
let enc = typeof encoding === 'function' ? null : enc;
let modifiedChunk = fn(chunk, enc, req, res);
index.js
Outdated
|
|
||
| // Encoding defaults to utf-8 as per the spec defined in the node.js docs. | ||
| // The encoding value can be overridden | ||
| res.set('Content-Length', Buffer.byteLength(modifiedChunk, encoding)); |
There was a problem hiding this comment.
res.write hook should not set the Content-Length.
index.js
Outdated
| // Encoding defaults to utf-8 as per the spec defined in the node.js docs. | ||
| // The encoding value can be overridden | ||
| res.set('Content-Length', Buffer.byteLength(modifiedChunk, encoding)); | ||
| return original.apply(res, [ modifiedChunk, [].slice.call(arguments, 1) ]) |
There was a problem hiding this comment.
pass the named arguments
|
Really like the new changes, much better approach! See the minor changes I'll like. |
index.js
Outdated
| const mungError = options.mungError; | ||
|
|
||
| res.write = function (chunk, encoding, callback) { | ||
| if (res.headersSent) { |
There was a problem hiding this comment.
res.write can be called after the headers are sent. Please remove the check.
There was a problem hiding this comment.
Hmm, that's true. Will update.
On that note, since res.write can be called multiple times before res.end, I'm thinking I'll also remove the 204 status check (we can't assume No content just because one call to res.write returns null).
index.js
Outdated
| res | ||
| ); | ||
|
|
||
| if (res.headersSent) { |
There was a problem hiding this comment.
res.write can be called after the headers are sent. Please remove the check.
…ove 204 response setting, minor formatting
No description provided.