feat(response): set content-type charset default to utf-8 #1416

[andersnylund]

Pre-Submission Checklist

• Opened an issue discussing these changes before opening the PR
• Ran the linter and tests via make prepush
• Included comprehensive and convincing tests for changes

Issues

Closes:

• Issue Default to utf-8 charEncoding #1416
• Issue #
• Issue #

Summarize the issues that discussed these changes

Changes

What does this PR do?

Makes a Response's content-type to default to charset=utf-8, if charSet() not specifically called

[retrohacker]

Oh snap! Awesome concise implementation and thorough tests 🎉

This is amazing @andersnylund, thank you ❤️

[retrohacker]

Also request @lrowe

[lrowe]

Choosing a default content type for a formatter should probably be the responsibility of the formatter, since binary content type should not have a charset.

Existing formatters are currently also tied to a particular charset since they set the Content-Length based on Buffer.byteLength(data) (Buffer.byteLength without a charset defaults to utf-8.) Example: https://github.com/restify/node-restify/blob/master/lib/formatters/json.js

(Setting Content-Length in the formatter should no longer be needed in recent NodeJS versions (>1.5.0), see: nodejs/node#1062.)

Perhaps it would make more sense to set the Content-Type header only after calling the formatter, providing the opportunity for the formatter to set a format appropriate default charset.

        const results = formatter(self.req, self, body);
        
        if (self._charSet) {
            type = type + '; charset=' + self._charSet;
        }

        // Update header to the derived content type for our formatter
        self.setHeader('Content-Type', type);

        // Finally, invoke the formatter and flush the request with it's results
        return flush(self, results);

[DonutEspresso]

I'm confused - I thought the content type indirectly specifies the formatter, not the other way around?

[lrowe]

@DonutEspresso while the content type picks the formatter the formatter should probably pick the charset if it is unspecified.

[lrowe]

Binary content types should not include a charset as it only makes sense for textual content types.

[andersnylund]

That is true. Didn't think of that. Having a list of different mime-types that should default to utf-8 is not convenient either keeping in mind all combinations. How about a blacklist of mime-types where not to format with utf-8?

[lrowe]

The list of binary types is also unbounded. I think it makes most sense to make the formatter to be responsible for picking a default / forcing a particular charset.

[lrowe]

The Python WebOb library uses an include list for setting a default charset:

* The ``default_charset`` is used as the default character set to return on
  the ``Content-Type`` header, if the ``Content-Type`` allows for a
  ``charset`` parameter. Currently the only ``Content-Type``'s that allow
  for a ``charset`` are defined to be: ``text/*``, ``application/xml``, and
  ``*/*+xml``. Any other ``Content-Type``'s will not have a ``charset``
  added.

https://github.com/Pylons/webob/blob/master/src/webob/response.py#L142

We'd probably need to add at least "application/javascript", "application/json", and "/+json" to that list.

Maintaining such a list does seem very inconvenient. Attaching the default to the formatter somehow seems more extensible.

[retrohacker]

Would this help? https://www.npmjs.com/package/mime-types

mime.charset('text/markdown') // 'UTF-8'

[lrowe]

Binary formatters will likely return a Buffer so perhaps we could do:

        const results = formatter(self.req, self, body);
        
        if (self._charSet) {
            type = type + '; charset=' + self._charSet;
        } else if (typeof results === "string") {
            type = type + '; charset=utf-8';
        }

        // Update header to the derived content type for our formatter
        self.setHeader('Content-Type', type);

        // Finally, invoke the formatter and flush the request with it's results
        return flush(self, results);

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically closed as stale because it has not had recent activity.