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

Add Symbol.species to ArrayConstructor, MapConstructor, SetConstructo… #18652

Merged
merged 1 commit into from
Nov 9, 2017

Conversation

NaridaL
Copy link
Contributor

@NaridaL NaridaL commented Sep 21, 2017

…r, ArrayBufferConstructor.

Fix Symbol.species in RegExpConstructor and PromiseConstructor.

See #2881 .

…r, ArrayBufferConstructor.

Fix Symbol.species in RegExpConstructor and PromiseConstructor.

See microsoft#2881 .
@NaridaL
Copy link
Contributor Author

NaridaL commented Sep 21, 2017

Symbol.species on TypedArrays still needs to be added, but it's implementation depends on whether i #18559 is accepted.

@microsoft microsoft deleted a comment from msftclas Sep 27, 2017
Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

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

Seems reasonable. @rbuckton should take a look here though.

@@ -150,7 +150,7 @@ interface Promise<T> {
}

interface PromiseConstructor {
readonly [Symbol.species]: Function;
readonly [Symbol.species]: PromiseConstructor;
Copy link
Member

Choose a reason for hiding this comment

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

Per the spec, the property type should be this.

Copy link
Member

Choose a reason for hiding this comment

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

@DanielRosenwasser, do you know if using a this type here would be problematic for someone writing a subclass?

@@ -202,7 +202,7 @@ interface RegExp {
}

interface RegExpConstructor {
[Symbol.species](): RegExpConstructor;
readonly [Symbol.species]: RegExpConstructor;
Copy link
Member

Choose a reason for hiding this comment

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

Per the spec, the property type should be this.

@@ -283,3 +283,16 @@ interface Float32Array {
interface Float64Array {
readonly [Symbol.toStringTag]: "Float64Array";
}

interface ArrayConstructor {
readonly [Symbol.species]: ArrayConstructor;
Copy link
Member

Choose a reason for hiding this comment

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

Per the spec, the property type should be this.

readonly [Symbol.species]: ArrayConstructor;
}
interface MapConstructor {
readonly [Symbol.species]: MapConstructor;
Copy link
Member

Choose a reason for hiding this comment

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

Per the spec, the property type should be this.

readonly [Symbol.species]: MapConstructor;
}
interface SetConstructor {
readonly [Symbol.species]: SetConstructor;
Copy link
Member

Choose a reason for hiding this comment

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

Per the spec, the property type should be this.

readonly [Symbol.species]: SetConstructor;
}
interface ArrayBufferConstructor {
readonly [Symbol.species]: ArrayBufferConstructor;
Copy link
Member

Choose a reason for hiding this comment

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

Per the spec, the property type should be this.

@NaridaL
Copy link
Contributor Author

NaridaL commented Oct 16, 2017

@rbuckton I changed it, but now I'm not so sure... according to the linked spec, returning this is the default behavior, but subclasses are free to change that behavior. The only requirement is that it be call-compatible with the original constructor. This would be better modeled with the first commit.

@rbuckton
Copy link
Member

@NaridaL, you make a good point. I need to discuss with @DanielRosenwasser. Ideally you want this, but that does prevent you from properly subclassing with a different value.

@rbuckton
Copy link
Member

After discussing with @DanielRosenwasser we should revert this back to the previous commit that used the explicit constructor type rather than this.

@NaridaL NaridaL force-pushed the symbol_species_add branch from 80f087c to 33344f9 Compare October 17, 2017 18:42
@NaridaL
Copy link
Contributor Author

NaridaL commented Oct 17, 2017

@rbuckton Done.

@mhegazy mhegazy merged commit 1408a4d into microsoft:master Nov 9, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
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.

6 participants