-
Notifications
You must be signed in to change notification settings - Fork 13.2k
this-related changes + new String.normalize overload in core.d.ts and es6.d.ts
#7664
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
Conversation
| * @param thisArg An object to which the this keyword can refer in the callbackfn function. If thisArg is omitted, undefined is used as the this value. | ||
| */ | ||
| some(callbackfn: (value: T, index: number, array: ReadonlyArray<T>) => boolean, thisArg?: any): boolean; | ||
| some(callbackfn: (value: T, index: number, array: this) => boolean, thisArg?: any): boolean; |
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.
this is going to be a breaking change, and not sure it adds much value here.
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.
//CC @RyanCavanaugh, thoughts?
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.
You mean just for ReadOnlyArray or all of them?
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.
i mean the use of this type in the call back argument type.
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.
Yes, I'm asking if it's a problem just for ReadOnlyArray or all the other Array classes as well where I did the same change?
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.
Impact is very low because support for fully subclassing Array is very recent and narrow (stable: only Edge). See Kangax:
http://kangax.github.io/compat-table/es6/#test-Array_is_subclassable_Array.prototype.filter
Only people who subclass Array and override one of those methods with callbacks (find, some, etc.) are impacted.
But in the future you can expect several frameworks to "hack" arrays for their own special purposes... At which point the annotation will become a tiny bit relevant (only a tiny bit because I never see the third parameter used in practice).
I will also say that TS has had breaking changes by way of tightening its type checks or fixing incorrect declarations at several occasions in the past.
I'd say: now is the right time for the change -- otherwise never.
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.
sorry for the delay. i meant all of them.
|
(I can update this PR for the es6.d.ts split in master once I get confirmation for mhegazy's comment.) |
|
sorry for they delay. if you can update the PR with the new file structure that would be great. thanks! |
|
So you don't want the change to make the callbacks have a |
|
See #8515 |
|
thanks!. do we still need this one then after #8515 is merged. |
|
No. |
thisinstead of explicit type.thisinstead of explicit type.formparameter as union of the allowed string literals.tests/cases/compiler/implementArrayInterfacestarted failing until I updated it, since it is now an error to implement array methods with old callback that acceptedT[]. Breaking change?tests/cases/compiler/duplicateOverloadInTypeAugmentationstill passed (with just baseline update for the types). But I've changed the overloads it defines fromT[]tothisanyway.jake runtestspasses.