lib Fix Part 2/6 – Array-related methods #50450

graphemecluster · 2022-08-25T18:24:43Z

General Information

PR separated out from #49855, because there might be some members expecting smaller PRs. As mentioned in the comments from the big PR, whether to review a single, large PR or 6 smaller PRs is up to the TypeScript Team to decide. I couldn't have found a better way for this; hopefully this will not bring any trouble to the Team.

This PR partially fixes #49773.
For details and the track list about the changes, please see #49773.

Part 2/6, Array-related methods

This is still a large PR, but I found no way to further split it because all the changes are related.

Fix #44191
Fix #45198
Fix #45199

graphemecluster · 2022-09-15T15:23:29Z

Actually this is not as complicated as you think (there are actually no breaking changes). The large amount of changes is really only due to %TypedArray%. Please also test this PR with TypeScript bot @sandersn.

yume-chan · 2022-09-27T13:10:09Z

src/lib/es5.d.ts

    valueOf(): Uint8ClampedArray;

    [index: number]: number;
 }

 interface Uint8ClampedArrayConstructor {
    readonly prototype: Uint8ClampedArray;
-    new(length: number): Uint8ClampedArray;
+    new(length?: number): Uint8ClampedArray;


I'm confused about these TypedArray types.

They are introduced in ES6 (ES2015), why are they in es5.d.ts? (Can we stop maintaining these old spec versions and merge them all)

This constructor without parameter is now declared in es2017.typedarrays.d.ts, but in es6 spec chapter 22.2.1.1 %TypedArray% ( ) they are already allowed.

sandersn · 2022-09-29T23:42:59Z

@typescript-bot run dt
@typescript-bot test top100
@typescript-bot user test this

typescript-bot · 2022-09-29T23:43:18Z

Heya @sandersn, I've started to run the diff-based user code test suite on this PR at 4bcd472. You can monitor the build here.

Update: The results are in!

typescript-bot · 2022-09-29T23:43:18Z

Heya @sandersn, I've started to run the parallelized Definitely Typed test suite on this PR at 4bcd472. You can monitor the build here.

typescript-bot · 2022-09-29T23:43:18Z

Heya @sandersn, I've started to run the diff-based top-repos suite on this PR at 4bcd472. You can monitor the build here.

Update: The results are in!

typescript-bot · 2022-09-29T23:57:25Z

@sandersn Here are the results of running the user test suite comparing main and refs/pull/50450/merge:

Everything looks good!

typescript-bot · 2022-09-30T00:44:44Z

@sandersn Here are the results of running the top-repos suite comparing main and refs/pull/50450/merge:

Something interesting changed - please have a look.

Details

excalidraw/excalidraw

2 of 5 projects failed to build with the old tsc and were ignored

tsconfig-types.json

error TS2344: Type 'ContextMenuOption' does not satisfy the constraint 'any[]'.
- src/components/ContextMenu.tsx#L128
error TS2339: Property 'push' does not exist on type 'ContextMenuOption'.
- src/components/ContextMenu.tsx#L131
error TS2339: Property 'length' does not exist on type 'ContextMenuOption'.
- src/components/ContextMenu.tsx#L134
error TS2322: Type 'ContextMenuOption' is not assignable to type 'ContextMenuOption[]'.
- src/components/ContextMenu.tsx#L139

tsconfig.json

error TS2344: Type 'ContextMenuOption' does not satisfy the constraint 'any[]'.
- src/components/ContextMenu.tsx#L128
error TS2339: Property 'push' does not exist on type 'ContextMenuOption'.
- src/components/ContextMenu.tsx#L131
error TS2339: Property 'length' does not exist on type 'ContextMenuOption'.
- src/components/ContextMenu.tsx#L134
error TS2322: Type 'ContextMenuOption' is not assignable to type 'ContextMenuOption[]'.
- src/components/ContextMenu.tsx#L139

react-hook-form/react-hook-form

2 of 3 projects failed to build with the old tsc and were ignored

tsconfig.json

error TS2322: Type '{ type: string; message: string; }' is not assignable to type 'Merge<FieldError, (Merge<FieldError, FieldErrorsImpl<{ action: string; }>> | undefined)[]>'.
- src/tests/type.test.tsx#L217
- src/tests/type.test.tsx#L250
error TS2322: Type '(data: { checkbox: string[]; }) => { errors: { checkbox?: { type: "error"; message: string; } | undefined; }; values: {}; }' is not assignable to type 'Resolver<{ checkbox: string[]; }, any>'.
- src/tests/useForm.test.tsx#L1404

sandersn · 2022-10-11T23:09:15Z

src/lib/es2015.core.d.ts


    /**
     * Returns a new array from a set of elements.
     * @param items A set of elements to include in the new array object.
     */
-    of<T>(...items: T[]): T[];
+    of<T extends any[]>(...items: T): T;


possible break: better typing of tuples, meaning more precise downstream errors.

sandersn · 2022-10-11T23:09:56Z

src/lib/es2015.core.d.ts

     * @param end If not specified, length of the this object is used as its default value.
     */
-    copyWithin(target: number, start: number, end?: number): this;
+    copyWithin(target: number, start: number, end?: number): T[];


possible break: subtypes of Array will now just be Array, missing any of the subtype methods.

Is the only reason for this change consistency?

This has to be long.

Returning this in {Array, %TypedArray%}.prototype.{fill, copyWithin, sort} seems to be correct, but TypeScript's type system is purely type-of-value-based, not instance-based. And because a subtype such as a tuple, or the generic parameters of a type, can contain the information of the values, returning this, which means an object with a same type, is technically incorrect for these methods which mutate the original object in a disruptive way. For example:

declare const foo: [42, 1, 2, 3]; const bar = foo.fill(42); // !!? interface ArraySubtype extends Array<string> { 0: "foo"; } const bar: ArraySubtype = ["foo", "bar", "baz"]; const bar1 = bar.sort(); // !!! interface Int8ArraySubtype extends Int8Array { 0: 42; } const baz = new Int8Array([42, 1, 2, 3]) as Int8ArraySubtype; const baz1 = baz.sort(); // ?!!

In fact, I'd done the some thing on RegExp (it's a deprecated method though):

TypeScript/src/lib/es5.d.ts

Line 988 in facef94

compile(pattern: string, flags?: string): RegExp;

I understand these are all rare cases, but since they return the same instance, people who find the original type desirable could just use the original thing. It just makes sense, and just because these are all rare cases it has to be corrected.

In contrast, the following is better typed this. That's why the this keyword in type level is misleading:

class Foo<T> { constructor(public foo: T) {} clone(): this { return Object.create(Object.getPrototypeOf(this), Object.getOwnPropertyDescriptors(this)); } } class Bar<T> extends Foo<T> { bar = true; } new Bar(42).clone(); // still a Bar<number>

A use case of the above:

interface Array<T> { slice(): this; }

(But then for ReadonlyArray, should the same method overload be typed this & T[]?)

(idea from #36554 (comment))

Is there a bug for this? Like the flatMap change in es2019, this isn't worth fixing unless it's breaking somebody's actual code, because any fix will break other code. Backward compatibility like this is the reason, for example, that we haven't been able to make overriding methods inherit parameter types.

sandersn · 2022-10-11T23:13:29Z

src/lib/es2015.core.d.ts

+     * order, until it finds one where predicate returns true. If such an element is found, find
+     * immediately returns that element value. Otherwise, find returns undefined.
+     * @param thisArg If provided, it will be used as the this value for each invocation of
+     * predicate. If it is not provided, undefined is used instead.


from the repl, this === global when not provided. I'd either leave off the last sentence, or reference the rules for this.

sandersn · 2022-10-11T23:14:01Z

src/lib/es2015.core.d.ts

+     * @param predicate find calls predicate once for each element of the array, in ascending
+     * order, until it finds one where predicate returns true. If such an element is found, find
+     * immediately returns that element value. Otherwise, find returns undefined.
+     * @param thisArg If provided, it will be used as the this value for each invocation of


Suggested change

* @param thisArg If provided, it will be used as the this value for each invocation of

* @param thisArg If provided, thisArg will be used as the this value for each invocation of

sandersn · 2022-10-11T23:18:42Z

src/lib/es2015.core.d.ts

+     * Returns the value of the first element in the array where predicate is true, and undefined
+     * otherwise.
+     * @param predicate find calls predicate once for each element of the array, in ascending
+     * order, until it finds one where predicate returns true. If such an element is found, find


Suggested change

* order, until it finds one where predicate returns true. If such an element is found, find

* order, until predicate returns true for some element. If such an element is found, find

I would personally drop the second sentence and change the last one to start "If no element is found,". The second sentence is redundant with the first, though, and redundancy can be useful for understanding, so it's fine if you want to keep it.

sandersn · 2022-10-11T23:32:52Z

src/lib/es2020.bigint.d.ts

@@ -182,7 +182,7 @@ interface BigInt64Array {
     * @param thisArg An object to which the this keyword can refer in the predicate function.
     * If thisArg is omitted, undefined is used as the this value.
     */
-    filter(predicate: (value: bigint, index: number, array: BigInt64Array) => any, thisArg?: any): BigInt64Array;
+    filter(predicate: (value: bigint, index: number, array: BigInt64Array) => unknown, thisArg?: any): BigInt64Array;


this probably won't break anything. But might!

I guess I would do it the other way round, i.e. changing all unknown to any then.

sandersn · 2022-10-11T23:35:06Z

src/lib/es2015.core.d.ts

+    find<S extends T>(predicate: (value: T, index: number, array: T[]) => value is S, thisArg?: any): S | undefined;
+
+    /**
+     * Returns the value of the first element in the array where predicate is true, and undefined


looking at the change to unknown in es2020.bigint reminds me that technically 'true' here is technically 'truthy'. I don't know if it's worth calling out in the text, though.

Yup, but this is another issue: whether there should be a syntax to allow returning anything in type predicates.

sandersn · 2022-10-11T23:38:03Z

src/lib/es2020.bigint.d.ts

@@ -271,7 +271,7 @@ interface BigInt64Array {
     * the accumulation. The first call to the callbackfn function provides this value as an argument
     * instead of an array value.
     */
-    reduce(callbackfn: (previousValue: bigint, currentValue: bigint, currentIndex: number, array: BigInt64Array) => bigint): bigint;
+    reduce(callbackfn: (previousValue: bigint, currentValue: bigint, currentIndex: number, array: BigInt64Array) => bigint, initialValue?: bigint): bigint;


this change makes sense, but I don't understand why this overload exists in the first place.

sandersn · 2022-10-11T23:48:13Z

src/lib/es5.d.ts

+    /**
+     * Returns the primitive value of the array.
+     */
+    valueOf(): T[];


possible (but unlikely) break since valueOf now has a more specific type

sandersn · 2022-10-11T23:49:01Z

src/lib/es5.d.ts

@@ -1245,8 +1249,7 @@ interface ReadonlyArray<T> {
     * @param callbackfn A function that accepts up to four arguments. The reduce method calls the callbackfn function one time for each element in the array.
     * @param initialValue If initialValue is specified, it is used as the initial value to start the accumulation. The first call to the callbackfn function provides this value as an argument instead of an array value.
     */
-    reduce(callbackfn: (previousValue: T, currentValue: T, currentIndex: number, array: readonly T[]) => T): T;
-    reduce(callbackfn: (previousValue: T, currentValue: T, currentIndex: number, array: readonly T[]) => T, initialValue: T): T;
+    reduce(callbackfn: (previousValue: T, currentValue: T, currentIndex: number, array: readonly T[]) => T, initialValue?: T): T;


I can't imagine how this will be a breaking change, but removing/adding overloads nearly always is somehow

BTW is #7014 not resolved also due to compatibility? It has been an issue for years.

sandersn · 2022-10-11T23:52:57Z

I marked what I think would be breaking changes. @andrewbranch Can you look at this for wording and possible breaks as well?

@RyanCavanaugh you might have opinions too.

graphemecluster · 2022-10-12T18:10:59Z

To what extent are breaking changes acceptable? I agree that reverting some of these changes reduces the risk of causing compatibility problems, but to address the issues, I still think some of them are unavoidable. The recent changes to RegExpMatchArray is a good example. An appropriate extent of breaking changes shall benefit TypeScript users the most (this could possibly be modelled by something like $\mathit{benefit}\left(x\right) = e^{-x^2}$, maybe lol, just a joke).

Anyway, I would like to wait until all reviewers reach a consensus on what must be reverted and what are acceptable before I commit changes to it.

graphemecluster · 2022-10-12T20:00:30Z

I notice that #47351 is an issue with Bug and Help Wanted tags (in contrast to those with Suggestion), and there exists non-breaking solutions while #36554 (comment) and #26976 (comment) seem to be the best. I guess you'd prefer a separate PR, but I'm happy to address it here.

sandersn · 2022-10-12T22:15:20Z

Definitely a separate PR. Smaller changes to the standard lib are better.

sandersn · 2022-12-17T00:01:20Z

To what extent are breaking changes acceptable?

I would say changes should target an improvement that feels like a "missing feature", or a fix for a specific bugs. Small improvements, like improved consistency or fixes for problems that nobody has noticed, are not worth it.

However, @RyanCavanaugh I'm sure has additional opinions and nuance.

Edit: Specifically, for this PR, fixing the three bugs in the description to add new methods is worthwhile, but needs to be done in a way that minimises changes elsewhere.

lib Types and Documentations Fix

1385bb7

typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Aug 25, 2022

graphemecluster mentioned this pull request Aug 25, 2022

lib Types and Documentations Fix #49855

Open

sandersn requested review from sandersn and weswigham and removed request for sandersn September 13, 2022 23:33

sandersn self-assigned this Sep 13, 2022

sandersn requested review from sandersn and RyanCavanaugh September 13, 2022 23:34

Merge main branch: Array line number changes

4bcd472

graphemecluster mentioned this pull request Sep 27, 2022

Add Array.prototype.findLast() and Array.prototype.findLastIndex() #49636

Merged

yume-chan reviewed Sep 27, 2022

View reviewed changes

graphemecluster changed the title ~~lib Types and Documentations Fix, Part 2/6~~ lib Fix Part 2/6 – Array-related methods Oct 9, 2022

Merge main branch: Array line number changes

9481bab

sandersn requested changes Oct 11, 2022

View reviewed changes

sandersn requested review from andrewbranch and removed request for weswigham October 11, 2022 23:53

typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Oct 31, 2022

Merge remote-tracking branch 'upstream/main' into implement49733-part2

06c6e5d

Merge remote-tracking branch 'upstream/main' into implement49733-part2

42c170b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib Fix Part 2/6 – Array-related methods #50450

lib Fix Part 2/6 – Array-related methods #50450

graphemecluster commented Aug 25, 2022 •

edited

Loading

graphemecluster commented Sep 15, 2022

yume-chan Sep 27, 2022 •

edited

Loading

sandersn commented Sep 29, 2022

typescript-bot commented Sep 29, 2022 •

edited

Loading

typescript-bot commented Sep 29, 2022 •

edited

Loading

typescript-bot commented Sep 29, 2022 •

edited

Loading

typescript-bot commented Sep 29, 2022

typescript-bot commented Sep 30, 2022

excalidraw/excalidraw

tsconfig-types.json

tsconfig.json

react-hook-form/react-hook-form

tsconfig.json

sandersn Oct 11, 2022

sandersn Oct 11, 2022

graphemecluster Oct 12, 2022

graphemecluster Oct 12, 2022

sandersn Dec 16, 2022

sandersn Oct 11, 2022

sandersn Oct 11, 2022

sandersn Oct 11, 2022

sandersn Oct 11, 2022

graphemecluster Oct 12, 2022

sandersn Oct 11, 2022

graphemecluster Oct 12, 2022

sandersn Oct 11, 2022

sandersn Oct 11, 2022

sandersn Oct 11, 2022

graphemecluster Oct 12, 2022

sandersn commented Oct 11, 2022

graphemecluster commented Oct 12, 2022

graphemecluster commented Oct 12, 2022

sandersn commented Oct 12, 2022

sandersn commented Dec 17, 2022 •

edited

Loading

	* @param thisArg If provided, it will be used as the this value for each invocation of
	* @param thisArg If provided, thisArg will be used as the this value for each invocation of

	* order, until it finds one where predicate returns true. If such an element is found, find
	* order, until predicate returns true for some element. If such an element is found, find

lib Fix Part 2/6 – Array-related methods #50450

Are you sure you want to change the base?

lib Fix Part 2/6 – Array-related methods #50450

Conversation

graphemecluster commented Aug 25, 2022 • edited Loading

General Information

Part 2/6, Array-related methods

graphemecluster commented Sep 15, 2022

yume-chan Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

sandersn commented Sep 29, 2022

typescript-bot commented Sep 29, 2022 • edited Loading

typescript-bot commented Sep 29, 2022 • edited Loading

typescript-bot commented Sep 29, 2022 • edited Loading

typescript-bot commented Sep 29, 2022

typescript-bot commented Sep 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Oct 11, 2022

graphemecluster commented Oct 12, 2022

graphemecluster commented Oct 12, 2022

sandersn commented Oct 12, 2022

sandersn commented Dec 17, 2022 • edited Loading

graphemecluster commented Aug 25, 2022 •

edited

Loading

yume-chan Sep 27, 2022 •

edited

Loading

typescript-bot commented Sep 29, 2022 •

edited

Loading

typescript-bot commented Sep 29, 2022 •

edited

Loading

typescript-bot commented Sep 29, 2022 •

edited

Loading

sandersn commented Dec 17, 2022 •

edited

Loading