Skip to content

Commit

Permalink
Change Headers multiple value handling for spec compatibility
Browse files Browse the repository at this point in the history
Consider this Headers object:

    var h = new Headers()
    h.append('accept', 'text/html')
    h.append('accept', 'text/plain')
    h.append('content-type', 'application/json')

Before:

- `h.get('accept')` returned `text/html`
- `h.getAll('accept')` returned an array of values
- `h.forEach` (and other iterables) did distinct iterations for each
  value of the same header

Now:

- `h.get('accept')` returns `text/html,text/plain`
- `h.getAll()` is no more
- `h.forEach` (and other iterables) now only do one iteration for each
  headers name, regardless of multiple values

This is in accordance with Section 3.1.2 of the spec, "combine" concept.

The updated tests currently break in Chrome and Firefox when exercising
the native implementation because their implementation is outdated. The
implementation in Edge is more correct, but the tests still fail there
because its implementation combines values with `, ` (notice the space)
rather than `,`.
  • Loading branch information
mislav committed Nov 10, 2016
1 parent 67627a4 commit 02b1dcb
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 45 deletions.
28 changes: 10 additions & 18 deletions fetch.js
Expand Up @@ -73,41 +73,33 @@
Headers.prototype.append = function(name, value) {
name = normalizeName(name)
value = normalizeValue(value)
var list = this.map[name]
if (!list) {
list = []
this.map[name] = list
}
list.push(value)
var oldValue = this.map[name]
this.map[name] = oldValue ? oldValue+','+value : value
}

Headers.prototype['delete'] = function(name) {
delete this.map[normalizeName(name)]
}

Headers.prototype.get = function(name) {
var values = this.map[normalizeName(name)]
return values ? values[0] : null
}

Headers.prototype.getAll = function(name) {
return this.map[normalizeName(name)] || []
name = normalizeName(name)
return this.has(name) ? this.map[name] : null
}

Headers.prototype.has = function(name) {
return this.map.hasOwnProperty(normalizeName(name))
}

Headers.prototype.set = function(name, value) {
this.map[normalizeName(name)] = [normalizeValue(value)]
this.map[normalizeName(name)] = normalizeValue(value)
}

Headers.prototype.forEach = function(callback, thisArg) {
Object.getOwnPropertyNames(this.map).forEach(function(name) {
this.map[name].forEach(function(value) {
callback.call(thisArg, value, name, this)
}, this)
}, this)
for (var name in this.map) {
if (this.map.hasOwnProperty(name)) {
callback.call(thisArg, this.map[name], name, this)
}
}
}

Headers.prototype.keys = function() {
Expand Down
38 changes: 11 additions & 27 deletions test/test.js
Expand Up @@ -122,8 +122,8 @@ suite('Headers', function() {
original.append('Content-Type', 'text/html')

var headers = new Headers(original)
assert.deepEqual(['application/json', 'text/plain'], headers.getAll('Accept'))
assert.deepEqual(['text/html'], headers.getAll('Content-Type'))
assert.equal(headers.get('Accept'), 'application/json,text/plain')
assert.equal(headers.get('Content-type'), 'text/html')
})
test('headers are case insensitive', function() {
var headers = new Headers({'Accept': 'application/json'})
Expand All @@ -141,9 +141,7 @@ suite('Headers', function() {
test('appends values to existing header name', function() {
var headers = new Headers({'Accept': 'application/json'})
headers.append('Accept', 'text/plain')
assert.equal(headers.getAll('Accept').length, 2)
assert.equal(headers.getAll('Accept')[0], 'application/json')
assert.equal(headers.getAll('Accept')[1], 'text/plain')
assert.equal(headers.get('Accept'), 'application/json,text/plain')
})
test('sets header name and value', function() {
var headers = new Headers()
Expand All @@ -167,20 +165,10 @@ suite('Headers', function() {
assert.isFalse(headers.has('Content-Type'))
assert.isNull(headers.get('Content-Type'))
})
test('returns list on getAll when header found', function() {
var headers = new Headers({'Content-Type': 'application/json'})
assert.isArray(headers.getAll('Content-Type'))
assert.equal(headers.getAll('Content-Type').length, 1)
assert.equal(headers.getAll('Content-Type')[0], 'application/json')
})
test('returns empty list on getAll when no header found', function() {
var headers = new Headers()
assert.isArray(headers.getAll('Content-Type'))
assert.equal(headers.getAll('Content-Type').length, 0)
})
test('converts field name to string on set and get', function() {
var headers = new Headers()
headers.set(1, 'application/json')
assert.isTrue(headers.has('1'))
assert.equal(headers.get(1), 'application/json')
})
test('converts field value to string on set and get', function() {
Expand All @@ -191,8 +179,8 @@ suite('Headers', function() {
assert.equal(headers.get('X-CSRF-Token'), 'undefined')
})
test('throws TypeError on invalid character in field name', function() {
assert.throws(function() { new Headers({'<Accept>': ['application/json']}) }, TypeError)
assert.throws(function() { new Headers({'Accept:': ['application/json']}) }, TypeError)
assert.throws(function() { new Headers({'<Accept>': 'application/json'}) }, TypeError)
assert.throws(function() { new Headers({'Accept:': 'application/json'}) }, TypeError)
assert.throws(function() {
var headers = new Headers()
headers.set({field: 'value'}, 'application/json')
Expand All @@ -209,10 +197,9 @@ suite('Headers', function() {
results.push({value: value, key: key, object: object})
})

assert.equal(results.length, 3)
assert.deepEqual({key: 'accept', value: 'application/json', object: headers}, results[0])
assert.deepEqual({key: 'accept', value: 'text/plain', object: headers}, results[1])
assert.deepEqual({key: 'content-type', value: 'text/html', object: headers}, results[2])
assert.equal(results.length, 2)
assert.deepEqual({key: 'accept', value: 'application/json,text/plain', object: headers}, results[0])
assert.deepEqual({key: 'content-type', value: 'text/html', object: headers}, results[1])
})
featureDependent(test, !nativeFirefox, 'forEach accepts second thisArg argument', function() {
var headers = new Headers({'Accept': 'application/json'})
Expand All @@ -229,7 +216,6 @@ suite('Headers', function() {

var iterator = headers.keys()
assert.deepEqual({done: false, value: 'accept'}, iterator.next())
assert.deepEqual({done: false, value: 'accept'}, iterator.next())
assert.deepEqual({done: false, value: 'content-type'}, iterator.next())
assert.deepEqual({done: true, value: undefined}, iterator.next())
})
Expand All @@ -240,8 +226,7 @@ suite('Headers', function() {
headers.append('Content-Type', 'text/html')

var iterator = headers.values()
assert.deepEqual({done: false, value: 'application/json'}, iterator.next())
assert.deepEqual({done: false, value: 'text/plain'}, iterator.next())
assert.deepEqual({done: false, value: 'application/json,text/plain'}, iterator.next())
assert.deepEqual({done: false, value: 'text/html'}, iterator.next())
assert.deepEqual({done: true, value: undefined}, iterator.next())
})
Expand All @@ -252,8 +237,7 @@ suite('Headers', function() {
headers.append('Content-Type', 'text/html')

var iterator = headers.entries()
assert.deepEqual({done: false, value: ['accept', 'application/json']}, iterator.next())
assert.deepEqual({done: false, value: ['accept', 'text/plain']}, iterator.next())
assert.deepEqual({done: false, value: ['accept', 'application/json,text/plain']}, iterator.next())
assert.deepEqual({done: false, value: ['content-type', 'text/html']}, iterator.next())
assert.deepEqual({done: true, value: undefined}, iterator.next())
})
Expand Down

0 comments on commit 02b1dcb

Please sign in to comment.