Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Removes string-key requirement; uses ToPropertyKey to coerce keys. Addresses #5. #10

Merged
merged 3 commits into from
Mar 9, 2018

Conversation

bathos
Copy link
Collaborator

@bathos bathos commented Mar 9, 2018

Follow up on changes discussed in #5 and also addresses comments about polyfill implementation in PR #8.

@ljharb, @zloirock

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Overall LGTM.

}

// Consistency with Map: object need not be technically iterable:
// Consistency with Map: contract is that entry has "0" and "1" keys, not
// that it is an array or iterable.

const { 0: key, 1: val } = pair;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn’t this throw if the array length isn’t 2, and if pair isn’t an array?

Copy link
Collaborator Author

@bathos bathos Mar 9, 2018

Choose a reason for hiding this comment

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

I followed the contract used by the Map constructor. Although all intrinsic APIs that yield entries do so as 2-member arrays, Map is the only existing consumer of them, and all it requires is that each member is an object — not even that the keys are actually present.

new Map([ {0: 'foo', 1: 'bar'}, { 1: 'baz' } ]);
// Map(2) {"foo" => "bar", undefined => "baz"}

Check out step 9 in 23.1.1.1 (Map Constructor)

(FWIW, this surprised me! But it seemed better not to introduce an inconsistency here.)

Copy link
Member

Choose a reason for hiding this comment

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

lol, agree to be consistent.

We might want to put the “extract a list of key/value from the entry” logic in an abstract operation so that both this and Map can share it.

@bathos bathos merged commit 808b7fc into master Mar 9, 2018
@bathos bathos deleted the coerce-keys-and-permit-symbols branch March 9, 2018 04:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants