Skip to content
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

Merged
merged 2 commits into from
Oct 23, 2018

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Oct 22, 2018

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.

@kitsonk kitsonk mentioned this pull request Oct 22, 2018
2 tasks
@kitsonk kitsonk changed the title Change DenoHeaders to be more idiomatic TypeScript [WIP] Change DenoHeaders to be more idiomatic TypeScript Oct 22, 2018
@kitsonk
Copy link
Contributor Author

kitsonk commented Oct 22, 2018

Based on the discussion on #1056 I am going to rework this to:

  • Make the private member a Symbol.
  • Create a mixin class to abstract the iterators.

@acconrad
Copy link
Contributor

@kitsonk just a thought here, according to what I've heard from other EcmaScript folks Web IDL private variables are best suited as WeakMaps especially if they need to be shared amongst other other implementation classes (in other words, if it's not truly private but a protected property)

js/fetch.ts Outdated Show resolved Hide resolved
@kitsonk
Copy link
Contributor Author

kitsonk commented Oct 22, 2018

@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).

@kitsonk kitsonk force-pushed the fix-iterators branch 2 times, most recently from 1cb3eba to 5061c65 Compare October 22, 2018 21:38
@kitsonk
Copy link
Contributor Author

kitsonk commented Oct 22, 2018

I am happy now though review from @kyranet and others would be appreciated.

@kitsonk kitsonk changed the title [WIP] Change DenoHeaders to be more idiomatic TypeScript Change DenoHeaders to be more idiomatic TypeScript Oct 22, 2018
Copy link
Contributor

@kevinkassimo kevinkassimo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

js/mixins/dom_iterable_test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@kyranet kyranet left a 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

js/mixins/dom_iterable_test.ts Outdated Show resolved Hide resolved
js/mixins/dom_iterable_test.ts Show resolved Hide resolved
js/mixins/dom_iterable_test.ts Show resolved Hide resolved
js/mixins/dom_iterable_test.ts Outdated Show resolved Hide resolved
js/mixins/dom_iterable.ts Outdated Show resolved Hide resolved
@kitsonk kitsonk force-pushed the fix-iterators branch 2 times, most recently from 6a2b88a to bb45cc7 Compare October 23, 2018 00:10
Copy link
Contributor

@kyranet kyranet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks!

@ry ry merged commit c0492ef into denoland:master Oct 23, 2018
ztplz added a commit to ztplz/deno that referenced this pull request Oct 25, 2018
@kitsonk kitsonk deleted the fix-iterators branch August 2, 2022 04:42
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.

None yet

6 participants