-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Change DenoHeaders to be more idiomatic TypeScript #1062
Conversation
Based on the discussion on #1056 I am going to rework this to:
|
@kitsonk just a thought here, according to what I've heard from other EcmaScript folks Web IDL private variables are best suited as |
@acconrad the private members will be "polyfillable" via WeakMaps. Here, I am not too sure there is value. What we are trying to do is keep things structurally compatible and avoid any potential name collisions. Because "private" isn't really "private" in TypeScript, it means that private member names leak to the public structure and can cause some assignability issues. A Symbol would effectively be ok, as it would be unique and not leak to the public structure, so I think it is better solution in this case (and we don't have to manage a WeakMap of private data, which gets a bit frustrating). Just to be pedantic, most OO languages allow private access across the same class instances and private fields when delivered in TC39 will have the same behaviour. Protected in traditional OO means that subsequent descendent classes would have access to the member, but not accessible externally, where as descendants don't have access to private (which holds true with both the WeakMaps and TC39 proposal). |
1cb3eba
to
5061c65
Compare
I am happy now though review from @kyranet and others would be appreciated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
5061c65
to
ec60a59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look really well, thanks you for this PR, it'll indeed reduce duplicated code in some Web APIs
6a2b88a
to
bb45cc7
Compare
bb45cc7
to
433a389
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks!
This PR changes the way the iterators are defined in
Headers
.It uses generators and yields up the values of the private
Map
. It also leverages the TypeScript type system to guard against values being passed into the constructor, which is where the value of TypeScript comes in. I think we should expect that if code is type safe in TypeScript that we shouldn't need to perform runtime checks.The assertion of
[object Iterator]
is something we shouldn't look to test. People should never depend on that for behaviours. Sniffing objects this way is very dangerous and not something we should ever promote. This does produce[object Generator]
and yes, the browser implementations produce[object Iterator]
but I am not sure why that matters.In just double checking things, I realise I do need to ensure that
new Headers(null)
fails though, as that is consistent with the browser behaviour.