Skip to content

Add Headers to HeadersInit #321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 6, 2017
Merged

Add Headers to HeadersInit #321

merged 1 commit into from
Nov 6, 2017

Conversation

mhegazy
Copy link
Contributor

@mhegazy mhegazy commented Nov 6, 2017

Missed this in #315

@mhegazy mhegazy merged commit 4d5f2a1 into master Nov 6, 2017
@mhegazy mhegazy deleted the addHeadersToHeadersInit branch November 6, 2017 16:56
@timocov
Copy link
Contributor

timocov commented Nov 6, 2017

It is not missed - I have did it specially because the spec does not have it :-)

@timocov
Copy link
Contributor

timocov commented Nov 6, 2017

For me this changes is a bit like change string[][] to [string, string][] - it is more correct and it will works but it does not correspond the spec.

@timocov
Copy link
Contributor

timocov commented Nov 6, 2017

@mhegazy so why this changes should be but [string, string][] not? :-)

@mhegazy
Copy link
Contributor Author

mhegazy commented Nov 6, 2017

record<ByteString, ByteString> is not really { [x:string]: string }. the later is much more restrictive than the earlier.

Header would be a valid record<ByteString, ByteString> but not a valid { [x:string]: string }. this was partially the behavior before the change by using object.

I did not want to go back to object, so just left it at Header which is a valid use case of this API, see https://developer.mozilla.org/en-US/docs/Web/API/Headers/Headers

@saschanaz
Copy link
Contributor

the later is much more restrictive than the earlier.

Indeed, new Headers(new Map([['abc', 'bcd']])).get('abc') returns bcd on Firefox and Chrome.

@saschanaz
Copy link
Contributor

saschanaz commented Nov 7, 2017

new Headers({ [Symbol.iterator]() { return new Map([['abc', 'bcd']]).entries() } }).get('abc') returns bcd so I think we can check iterator type rather than adding Headers?

PS: Filed an issue for Edge returning null.

@saschanaz
Copy link
Contributor

saschanaz commented Nov 7, 2017

type Record<K, V> = { [Symbol.iterator](): IterableIterator<[K, V]> } | { [x: K]: V };

This should also work for URLSearchParams receiving records.

new URLSearchParams({ [Symbol.iterator]() { return new Map([['abc', 'bcd']]).entries() } }).toString()
// returns 'abc=bcd'

@mhegazy
Copy link
Contributor Author

mhegazy commented Nov 7, 2017

Headers is not iterable in --target ES5.. not sure what is the best way here. we can go back to object..

@saschanaz
Copy link
Contributor

Filed microsoft/TypeScript#19806.

@timocov
Copy link
Contributor

timocov commented Nov 7, 2017

which is a valid use case of this API, see https://developer.mozilla.org/en-US/docs/Web/API/Headers/Headers

But it is not spec :)

Nobody known the truth: MDN has no case with string[][] as Headers ctor params, whatwg - Headers as Headers ctor params (and it is very strange) - I guess we need to use intersection of these specs and pass only { [key: string]: string } as valid Headers ctor params :)

@mhegazy
Copy link
Contributor Author

mhegazy commented Nov 7, 2017

I guess we need to use intersection of these specs and pass only { [key: string]: string } as valid Headers ctor params :)

that is not correct.. A Header is not { [key:string]: string }, and so is a Map<string, string>. which are all valid use cases of this API.

@timocov
Copy link
Contributor

timocov commented Nov 7, 2017

record<ByteString, ByteString> is not really { [x:string]: string }. the later is much more restrictive than the earlier.

@mhegazy could you please provide some examples to understand?

A Header is not { [key:string]: string }

Under as valid Headers ctor params I mean HeadersInit not Headers

@mhegazy
Copy link
Contributor Author

mhegazy commented Nov 7, 2017

Here is an example : https://github.com/firebase/firebase-js-sdk/blob/0a6ad04befd820ec52b05040e86ff52a62e37fa5/packages/messaging/src/models/token-manager.ts#L173-L198

Header should be a valid header value. but Header is not assignable to { [x:string]: string }.

@timocov
Copy link
Contributor

timocov commented Nov 7, 2017

@mhegazy Sorry, but I still cannot recognize the difference.

The spec says that Headers might be constructed from HeadersInit value.
HeadersInit in its turn should be sequence<sequence<ByteString>> or record<ByteString, ByteString>.
sequence<T> is lists of values of type T.
record<K, V> is ordered map with keys that are instances of K and values that are instances of V.

According to this I have a few questions:

  1. Why you say that record<ByteString, ByteString> is not really { [x:string]: string }? Of course it should be { [key: ByteString]: ByteString } (or even Map<ByteString, ByteString> also) but seems that it is not important for now.

Header would be a valid record<ByteString, ByteString> but not a valid { [x:string]: string }.

What do you mean under Header: Headers or HeadersInit or something else (according the spec)?

  1. What is valid header value?

Please correct me if I am somewhere wrong.

@mhegazy
Copy link
Contributor Author

mhegazy commented Nov 7, 2017

record<K,V> talks about own properties.. does not look at methods.. {[x:string]: string} applies to all members of the type, methods or properties.. that is why Header is not {[x:string]: string} in TS but is record<string, string> in the spec

@timocov
Copy link
Contributor

timocov commented Nov 9, 2017

Oh, I get it, thank you!

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.

3 participants