Skip to content

Conversation

nitin-is-me
Copy link

Summary

Refactored res.send() in response.js to improve readability and maintainability. Broke up the large method into smaller helper functions:

  • prepareChunk
  • setETagHeader
  • stripBodyIfNotAllowed

Behavior is preserved 100%. This helps future contributors understand and modify res.send() more easily.


Copy link

@Hardanish-Singh Hardanish-Singh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@bjohansebas bjohansebas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution. At the moment, I’m a bit unsure if this would be a good idea. I don’t think it would improve the code’s readability or its maintenance much. I’m going to block it until more people on the team can review it

Comment on lines +134 to +135
if (!this.get('Content-Type')) this.type('html');
encoding = 'utf8';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using curly braces would make it more consistent and readable.

Comment on lines +136 to +137
} else if (typeof chunk === 'boolean' || typeof chunk === 'number' || chunk === null) {
chunk = chunk === null ? '' : chunk;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make sense to split the null case from the boolean | number case to avoid using a ternary.

const etagFn = app.get('etag fn');
const shouldGenerateETag = !res.get('ETag') && typeof etagFn === 'function';

if (chunk === undefined) return chunk;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could even return earlier.

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

Successfully merging this pull request may close these issues.

4 participants