jshttp / negotiator

An HTTP content negotiator for Node.js
MIT License
314 stars 34 forks source link

Sort by precedence in Accept header if priorities are equal #14

Closed xaka closed 10 years ago

jonathanong commented 10 years ago

is this a spec? i think order doesn't matter in the spec, but the q value does. i'd personally rather have the values be returned in the user-provided values than by the client-provided values.

federomero commented 10 years ago

I'm pretty sure there's nothing on the specs that says that the order of appearance matters. It would be nice to know if this is expected in practice.

Do you have any specific need for this?

madbence commented 10 years ago

This is definitely a problem:

var express = require('express');

var app = express();

app
  .route('/')
  .get(function(req, res) {
    res.format({
      json: function() { res.send('json'); },
      html: function() { res.send('html'); }
    });
  });

app.listen(8000);

Chrome sends Accept: text/html, application/xhtml+xml, */*;q=0.8, and the response has correct type:

HTTP/1.1 200 OK
X-Powered-By: Express
Vary: Accept
Content-Type: text/html; charset=utf-8
Content-Length: 4
ETag: W/"4-410646757"
Date: Wed, 11 Jun 2014 11:22:58 GMTConnection: keep-alive

IE (9, 10) sends Accept: text/html, application/xhtml+xml, */*, and the response type is definitely wrong.

neg-tmp λ curl -I -H "Accept: text/html, application/xhtml+xml, */*" localhost:8000
HTTP/1.1 200 OK
X-Powered-By: Express
Vary: Accept
Content-Type: application/json; charset=utf-8
Content-Length: 4
ETag: W/"4-1795630405"
Date: Wed, 11 Jun 2014 11:23:11 GMT
Connection: keep-alive

So res.format is currently broken in express.

According to the spec, the order of appearance doesn't matter, but more specific types should have higher precedence (see RFC2616 section 14.1).

var n = require('negotiator/lib/mediaType')
n('text/html, application/xhtml+xml, */*', ['application/json', 'text/html']);
// [ 'application/json', 'text/html' ]
// but it should be
// [ 'text/html', 'application/json' ]

A temporary workaround is to order res.format entries by precedence.

federomero commented 10 years ago

Hi @madbence, thank you for reporting this.

I agree this is not working correctly but I don't think it's the same issue that was originally submitted, this has nothing to do with the order that items appear on the accept header.

madbence commented 10 years ago

Sure, you're right, I've created a new issue for this

dougwilson commented 10 years ago

@madbence I saw this change and the new version of this module is making it way up the dep chain right now for an express destination ;)