feat: add support for non-strict formatters #1721

misterdjules · 2018-11-19T23:45:02Z

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

I did not create an issue prior to submitting this PR, as I thought looking at the rationale for this change without looking at the actual code change could be confusing and less productive. Please let me know if you'd still like me to open an issue.

Summarize the issues that discussed these changes

Changes

What does this PR do?

This PR makes res.send not override any content-type that was previously set on the response even when no formatter can be found for that content-type.

The goal of this PR is to improve developers' experience. Silently overriding the content-type of the response when no formatter is present seems implicit and surprising. I think we should aim for behavior that is less surprising.

One consequence of this PR is that it decouples the content-type from formatters as long as the content-type is set explicitly (e.g. using res.setHeader('content-type', ...) or res.contentType = ...). In those cases, it effectively makes formatters optional.

If no content-type is set explicitly on the response, the request would still result in an error if content-type negotiation failed to match the set of accepted content types from the client's request's accept header and the set of formatters available to the server.

Another approach I considered was to make res.send return or throw an error in this case and having users who run into that issue use res.sendRaw. However it seems that while that behavior would be even more explicit, the current approach seems more flexible and natural for at least some of our partners.

For instance, the approach implemented in this PR is what some developers at Netflix seem to expect from a HTTP framework. Using res.sendRaw feels very unnatural to them and would require that they write linting rules to catch when their developers use res.send instead of res.sendRaw. This change would allow them to get rid of the no-op formatter they wrote to work around this issue while still using res.send.

@restify/core I'd like to read your thoughts on this.

cprussin · 2018-11-19T23:57:40Z

For those who are not able to access the internal Netflix repo linked above, the no-op formatter looks like this:

const formatter = (req, res, body) => body;

...

restify.createServer({
    formatters: {
        'text/html': formatter,
        'text/css': formatter,
        ...
    }
});

It's not a great ergonomic, because if a developer forgets to add a line for each content-type they want to send, and don't remember to use sendRaw, then the content-type will be overwritten by restify. This can be hard for folks to debug since they typically don't suspect the server to be changing the content type they set, and remembering to use sendRaw is unnatural, especially with developers who are used to other frameworks like express.

hekike

Please add a 7to8 guide to here:
https://github.com/restify/node-restify/tree/master/docs/guides

micahr · 2018-11-27T04:46:48Z

I'm a 👍 for this change. Definitely don't want to magically pick a content-type without developer input. An additional option would be to allow a developer to specify a fallback or default content-type at initialization time that would then be used if there wasn't anything else specified on the request.

DonutEspresso · 2018-11-28T22:25:31Z

docs/index.md

+If you're using `res.send()` restify will determine the content-type to respond
+with by:
+
+1. negotiating the content-type by matching available formatters with the


Should we reorder this list to reflect the order of lookup in the code?

The way I ordered it is by order of priority, which is the reverse of the order in which the code runs those checks. Does that make sense?

If so would you still prefer to order it the same way the code performs those checks or should I keep it the way it is and mention that it's ordered by priority?

Ah, I see now, I was expecting it the other way around. Maybe a note to say "ascending priority" for clarity, then?

DonutEspresso · 2018-11-28T22:30:57Z

lib/response.js

        self.setHeader('Content-Type', type);

+        if (!formatter) {
+            return flush(self, body);


I was wondering if we should assert on this unformatted body to ensure it is either string/buffer, but I realized we call flush() in many places so it may be outside of the scope of this PR. I would expect res.write to throw in that scenario, but it may be better if we can catch it earlier and provide a better error message.

That's a good point. We're already asserting on that in the sendRaw case, so we might as well do the same as you suggest in this case.

DonutEspresso · 2018-11-29T00:29:30Z

Apologies, one more q - does this effectively remove the need for sendRaw?

misterdjules · 2018-11-30T01:33:32Z

@DonutEspresso

Apologies, one more q - does this effectively remove the need for sendRaw?

I don't think it does. With this change res.sendRaw's use case would still be about:

not applying any formatter at all to the response, whether it's optional or not
not performing any content-type negotiation (not inferring the response's content-type based on the request's accept header if content-type is not set)

misterdjules · 2018-11-30T01:38:33Z

@DonutEspresso I just pushed a second commit that changes the logic in res.__send a little bit so that we assert on the type of the unformatted body both when:

res.__send is called through res.sendRaw
res.__send is called through res.send but not formatter is found

Let me know if that looks better to you!

misterdjules · 2018-11-30T01:46:00Z

@cprussin Would it be worth it for you to test in a branch of your application that this PR addresses your use cases before we merge it?

DonutEspresso · 2018-11-30T02:04:03Z

lib/response.js

-        return flush(self, formatter(self.req, self, body));
+        // if no formatting, assert that the value to be written is a string or
+        // a buffer, then send it.
+        assert.ok(


I just realized that the formatters don't necessarily enforce a proper return value, so could we benefit from moving this assertion into flush()?

@DonutEspresso Done in the commit I just pushed (force-pushed unintentionally, sorry for that).

misterdjules · 2018-11-30T21:50:29Z

Another approach that I haven't mentioned so far would be to "rename" res.sendRaw to res.send and rename res.send to res.sendFormatted, or res.send(code, body, headers, {formatted: true}), and have the "formatted version" of res.send be more strict when formatters are missing.

@restify/core Any preference between the approach currently implemented in this PR (making formatters optional and their usage implicit) and the one I just mentioned (making using formatters more explicit)?

I don't have a strong preference either way, but I thought I'd make sure we present various approaches before committing, as those changes are all breaking.

misterdjules · 2018-12-05T19:24:37Z

Update on current status: had a discussion with @DonutEspresso and @mridgway and we decided to make this change non-breaking and opt-in via a flag passed at server creation time. This will:

Avoid the need for cutting a new major release of restify (by definition)
Allow consumers to opt into this behavior rather than force them to transition.

If/when we're confident this new behavior is suitable for all consumers, we can propose to enable it by default as a breaking change.

…matter found

misterdjules · 2018-12-06T18:45:00Z

@restify/core This is ready for another (hopefully final) round of review. Thanks for your patience and my apologies for the unnecessary churn on that one 🙏

@hekike Since this is no longer a breaking change, we don't need a migration guide anymore.

DonutEspresso

Thanks, this looks good! One thought - because it is possible to have both formatters and optionalFormatters defined, I found it confusing at first to think about what the end behavior would be. At first I thought it might mean that formatters are skipped entirely, but that's not the case.

Would it be less confusing if the option name was strictFormatters, which defaults to true and enforces the current RFC behavior?

DonutEspresso · 2018-12-06T20:21:58Z

lib/response.js

@@ -346,6 +348,11 @@ function patch(Response) {
            };
        }

+        assert.ok(


DonutEspresso · 2018-12-06T20:30:09Z

test/formatter-optional.test.js

+    // res.send still sends a successful response even when a formatter is not
+    // set up for a specific content-type.
+    SERVER.get('/11', function handle(req, res, next) {
+        console.log('got request');


Should this be removed? One in the subsequent test too.

Ah yes good catch :)

misterdjules · 2018-12-06T21:08:50Z

@DonutEspresso

Would it be less confusing if the option name was strictFormatters, which defaults to true and enforces the current RFC behavior?

strictFormatters definitely works too, I went back and forth between those two when putting those changes together.

I went for optionalFormatters because by default it is false, and one has to opt into it by setting the value to true, which I found less confusing at the time. I like the name strictFormatters better though, so I'm totally OK with making that change.

misterdjules · 2018-12-06T21:35:07Z

@DonutEspresso I believe I addressed your latest comments, do you mind taking another look? Thanks!

DonutEspresso · 2018-12-06T21:59:55Z

🎉

misterdjules requested review from hekike, DonutEspresso, yunong and trentm November 19, 2018 23:55

hekike requested changes Nov 20, 2018

View reviewed changes

misterdjules requested review from daviande, micahr, rajatkumar and retrohacker November 20, 2018 18:06

micahr approved these changes Nov 27, 2018

View reviewed changes

DonutEspresso approved these changes Nov 28, 2018

View reviewed changes

DonutEspresso reviewed Nov 28, 2018

View reviewed changes

DonutEspresso reviewed Nov 30, 2018

View reviewed changes

misterdjules force-pushed the res-send-no-content-type-override branch from c282890 to 7af4e37 Compare November 30, 2018 21:27

Julien Gilli added 2 commits December 5, 2018 14:40

BREAKING CHANGE: do not override content-type in res.send

639b489

assert on unformatted body including in the res.send case when no for…

612a184

…matter found

misterdjules force-pushed the res-send-no-content-type-override branch from 7af4e37 to 05843fa Compare December 6, 2018 00:30

misterdjules changed the title ~~BREAKING CHANGE: do not override content-type in res.send~~ make formatters optional when sending response Dec 6, 2018

misterdjules changed the title ~~make formatters optional when sending response~~ feat: make formatters optional when sending response Dec 6, 2018

refactor to make changes non-breaking

20566cf

misterdjules force-pushed the res-send-no-content-type-override branch from 05843fa to 20566cf Compare December 6, 2018 00:40

DonutEspresso reviewed Dec 6, 2018

View reviewed changes

Julien Gilli added 2 commits December 6, 2018 13:15

remove leftover debug logs

17a12cd

optionalFormatters -> strictFormatters

df57d72

hekike approved these changes Dec 6, 2018

View reviewed changes

misterdjules changed the title ~~feat: make formatters optional when sending response~~ feat: add support for non-strict formatters Dec 7, 2018

misterdjules merged commit de1833a into master Dec 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for non-strict formatters #1721

feat: add support for non-strict formatters #1721

misterdjules commented Nov 19, 2018 •

edited

Loading

cprussin commented Nov 19, 2018

hekike left a comment

micahr commented Nov 27, 2018

DonutEspresso Nov 28, 2018

misterdjules Nov 30, 2018

DonutEspresso Nov 30, 2018

DonutEspresso Nov 28, 2018

misterdjules Nov 30, 2018

DonutEspresso commented Nov 29, 2018

misterdjules commented Nov 30, 2018

misterdjules commented Nov 30, 2018

misterdjules commented Nov 30, 2018

DonutEspresso Nov 30, 2018

misterdjules Nov 30, 2018

misterdjules Nov 30, 2018

misterdjules commented Nov 30, 2018

misterdjules commented Dec 5, 2018 •

edited

Loading

misterdjules commented Dec 6, 2018

DonutEspresso left a comment

DonutEspresso Dec 6, 2018

DonutEspresso Dec 6, 2018

misterdjules Dec 6, 2018

misterdjules commented Dec 6, 2018

misterdjules commented Dec 6, 2018

DonutEspresso commented Dec 6, 2018

feat: add support for non-strict formatters #1721

feat: add support for non-strict formatters #1721

Conversation

misterdjules commented Nov 19, 2018 • edited Loading

Pre-Submission Checklist

Issues

Changes

cprussin commented Nov 19, 2018

hekike left a comment

Choose a reason for hiding this comment

micahr commented Nov 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonutEspresso commented Nov 29, 2018

misterdjules commented Nov 30, 2018

misterdjules commented Nov 30, 2018

misterdjules commented Nov 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misterdjules commented Nov 30, 2018

misterdjules commented Dec 5, 2018 • edited Loading

misterdjules commented Dec 6, 2018

DonutEspresso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misterdjules commented Dec 6, 2018

misterdjules commented Dec 6, 2018

DonutEspresso commented Dec 6, 2018

misterdjules commented Nov 19, 2018 •

edited

Loading

misterdjules commented Dec 5, 2018 •

edited

Loading