[BUGFIX release] Correct types for Ember Arrays #20256

wagenet · 2022-11-07T17:42:14Z

This PR does a couple of things to improve array types:

More fully unifies internal array types with external preview types.
Corrects NativeArray types to not override methods from the true native JS array.

Fixes #20230

chriskrycho · 2022-11-07T19:07:38Z

Top-level request (I'll review further later): can you add an explanation to the PR so we can refer to it later to understand why these specific changes are correct?

chriskrycho · 2022-11-07T19:20:04Z

packages/@ember/-internals/utils/types.ts

+export type MethodsOf<O> = {
+  [K in keyof O]: O[K] extends AnyFn ? O[K] : never;
+};


This doesn't actually work, unfortunately. You need something like the final keyof as used in MethodNamesOf below to make it properly strip out the bits which are actually never. It turns out the easiest way to get MethodsOf is to reuse MethodNamesOf in conjunction with Pick:

type MethodsOf<O> = Pick<O, MethodNamesOf<O>>;

playground

Suggested change

export type MethodsOf<O> = {

[K in keyof O]: O[K] extends AnyFn ? O[K] : never;

};

export type MethodsOf<O> = Pick<O, MethodNamesOf<O>>;

packages/@ember/array/index.ts

wagenet · 2022-11-07T19:20:54Z

types/preview/@ember/array/mutable.d.ts

@@ -15,7 +15,7 @@ declare module '@ember/array/mutable' {
    /**
     * __Required.__ You must implement this method to apply this mixin.
     */
-    replace(idx: number, amt: number, objects: T[]): this;
+    replace(idx: number, amt: number, objects: T[]): void;


In some cases we do return this, but the docs state that we do not return.

packages/@ember/array/index.ts

wagenet · 2022-11-07T19:27:31Z

packages/@ember/array/index.ts

@@ -535,7 +540,7 @@ interface EmberArray<T> extends Enumerable {
    @return {Object} receiver
    @public
  */
-  setEach<K extends string>(key: K, value: Value<T, K>): this;
+  setEach<K extends keyof T>(key: K, value: T[K]): this;


In theory we can set compound keys, but I feel like we probably shouldn't.

wagenet · 2022-11-07T19:28:31Z

packages/@ember/array/index.ts

-  find<S extends T, Target = void>(
-    predicate: (this: void, value: T, index: number, obj: T[]) => value is S,
-    thisArg?: Target
-  ): S | undefined;


Just moved this.

wagenet · 2022-11-07T19:29:02Z

packages/@ember/array/index.ts

+  invoke<M extends MethodNamesOf<T>>(
+    methodName: M,
+    ...args: MethodParams<T, M>
+  ): NativeArray<MethodReturns<T, M>>;


The public preview types were better here.

wagenet · 2022-11-07T19:43:41Z

type-tests/preview/@ember/array-test/array.ts

@@ -26,13 +27,11 @@ expectTypeOf(people.objectAt(0)).toEqualTypeOf<Person | undefined>();
 expectTypeOf(people.objectsAt([1, 2, 3])).toEqualTypeOf<Array<Person | undefined>>();

 expectTypeOf(people.filterBy('isHappy')).toMatchTypeOf<Person[]>();
-expectTypeOf(people.filterBy('isHappy')).toMatchTypeOf<MutableArray<Person>>();
+expectTypeOf(people.filterBy('isHappy')).toMatchTypeOf<NativeArray<Person>>();


NativeArray !== MutableArray

wagenet · 2022-11-07T19:44:02Z

type-tests/preview/ember/ember-module-tests.ts

@@ -153,7 +153,7 @@ expectTypeOf(Ember.Application.create()).toEqualTypeOf<Ember.Application>();
 expectTypeOf(new Ember.ApplicationInstance()).toEqualTypeOf<Ember.ApplicationInstance>();
 expectTypeOf(Ember.ApplicationInstance.create()).toEqualTypeOf<Ember.ApplicationInstance>();
 // Ember.Array
-const a1: Ember.Array<string> = Ember.A([]);
+const a1: Ember.NativeArray<string> = Ember.A([]);


NativeArray !== EmberArray

wagenet · 2022-11-07T19:44:44Z

types/preview/@ember/array/-private/native-array.d.ts

+   * The final definition of NativeArray removes all native methods. This is the list of removed methods
+   * when run in Chrome 106.
+   */
+  type IGNORED_MUTABLE_ARRAY_METHODS =


The below should be the same as internal.

wagenet · 2022-11-07T19:45:02Z

types/preview/@ember/array/index.d.ts

@@ -4,6 +4,9 @@ declare module '@ember/array' {
  import Enumerable from '@ember/array/-private/enumerable';
  import NativeArray from '@ember/array/-private/native-array';

+  // We don't currently attempt to fully type-check compound keys
+  type CompoundPropertyKey<T> = `${keyof T & string}.${string}`;


Bring these more in line with internal types.

wagenet · 2022-11-07T19:45:22Z

types/preview/@ember/array/mutable.d.ts

@@ -45,12 +45,12 @@ declare module '@ember/array/mutable' {
     * Pop object from array or nil if none are left. Works just like `pop()` but
     * it is KVO-compliant.
     */
-    popObject(): T;
+    popObject(): T | null | undefined;


These types were incorrect before.

chriskrycho · 2022-11-09T16:35:16Z

packages/@ember/array/index.ts

@@ -792,6 +790,10 @@ interface EmberArray<T> extends Enumerable {
    @return {Object} Found item or `undefined`.
    @public
  */
+  find<S extends T, Target = void>(


Note: conventionally/idiomatically it's U in a case like this (since T → U in the alphabet). Not a thing I care about changing, to be sure, just worth noting!

I think I just copied this from somewhere else :)

chriskrycho · 2022-11-09T16:36:12Z

packages/@ember/array/index.ts

+/**
+ * The final definition of NativeArray removes all native methods. This is the list of removed methods
+ * when run in Chrome 106.
+ */


Note for posterity/future readers looking at this PR historically—we definitely need this, but we need even more to just get the heck rid of it, since this could in principle change dynamically over time.

chriskrycho · 2022-11-09T17:19:08Z

packages/@ember/array/type-tests/mutable.ts


 expectTypeOf(arr).toMatchTypeOf<MutableArray<Foo>>();

 expectTypeOf(arr.replace(1, 1, [foo])).toEqualTypeOf<void>();
-// TODO: Why doesn't this fail?
+// @ts-expect-error invalid item


It's nice that all of these fail now the way we want them to. 👍🏼

I think we just needed a better test item :)

chriskrycho

Couple of tweaks needed, but this is a really nice step forward. 💙

chriskrycho · 2022-11-09T17:21:26Z

packages/@ember/debug/container-debug-adapter.ts

+  catalogEntriesByType(type: string): string[] {
+    let namespaces = Namespace.NAMESPACES;
+    let types: string[] = [];


Hmm. Documentation is very apt to be incorrect in this space, though; folks who wrote the docs likely were not thinking about the differences (because the differences are subtle!). Let's avoid changing the behavior here—we can always do so in a later PR, but keeping types changes separate from behavior changes will help us avoid snafus.

chriskrycho · 2022-11-09T17:22:14Z

packages/@ember/debug/data-adapter.ts

    let containerDebugAdapter = this.containerDebugAdapter;

    let stringTypes = containerDebugAdapter.canCatalogEntriesByType('model')
      ? containerDebugAdapter.catalogEntriesByType('model')
      : this._getObjectsOnNamespaces();

    // New adapters return strings instead of classes.
-    let klassTypes = emberA(stringTypes).map((name) => {
+    let klassTypes = stringTypes.map((name) => {


On the one hand, this is definitely a change I'm excited to be able to make; on the other, my note here is the same as above. Let's do it in a separate PR so it's easy to trace back to if for some reason it ends up being an issue for folks.

chriskrycho · 2022-11-09T17:22:23Z

packages/@ember/debug/data-adapter.ts

+    let namespaces = Namespace.NAMESPACES;
+    let types: string[] = [];


Same here, presumably.

1. Using the keyof constraint feature introduced in 4.1, avoid ever introducing types with properties whose type is `never` for the non-method fields. 2. Handle recursively applying this constraint to nested methods for the `EmberMethodsOf` type, which has additional complexity to handle Ember's string-based dispatch. Combined with (1), this actually *simplifies* the types, eliminating one layer of conditional types.

wagenet requested a review from chriskrycho November 7, 2022 17:42

wagenet force-pushed the native-array-fixes branch from 86ce43b to fa78d49 Compare November 7, 2022 18:09

wagenet force-pushed the native-array-fixes branch 3 times, most recently from 5c8555b to cfaf5d5 Compare November 7, 2022 19:39

chriskrycho reviewed Nov 7, 2022

View reviewed changes

packages/@ember/array/index.ts Outdated Show resolved Hide resolved

chriskrycho reviewed Nov 7, 2022

View reviewed changes

packages/@ember/array/index.ts Outdated Show resolved Hide resolved

chriskrycho reviewed Nov 7, 2022

View reviewed changes

packages/@ember/array/index.ts Outdated Show resolved Hide resolved

chriskrycho reviewed Nov 7, 2022

View reviewed changes

packages/@ember/array/index.ts Outdated Show resolved Hide resolved

wagenet force-pushed the native-array-fixes branch from 4513a24 to 3a1c614 Compare November 7, 2022 19:42

wagenet commented Nov 7, 2022

View reviewed changes

wagenet force-pushed the native-array-fixes branch 2 times, most recently from a6735ca to e3d387e Compare November 7, 2022 21:25

chriskrycho reviewed Nov 9, 2022

View reviewed changes

chriskrycho requested changes Nov 9, 2022

View reviewed changes

chriskrycho changed the title ~~Improve types for Ember Arrays~~ [BUGFIX release] Correct types for Ember Arrays Nov 9, 2022

chriskrycho added Bug TypeScript Work on Ember’s types labels Nov 9, 2022

Improve types for Ember Arrays

720c302

wagenet force-pushed the native-array-fixes branch from 3623985 to 02c9eed Compare November 9, 2022 19:46

chriskrycho force-pushed the native-array-fixes branch from 02c9eed to c1c9a47 Compare November 9, 2022 19:55

chriskrycho force-pushed the native-array-fixes branch from c1c9a47 to 072830a Compare November 9, 2022 20:09

chriskrycho approved these changes Nov 9, 2022

View reviewed changes

chriskrycho enabled auto-merge November 9, 2022 20:11

chriskrycho merged commit fae849c into emberjs:master Nov 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUGFIX release] Correct types for Ember Arrays #20256

[BUGFIX release] Correct types for Ember Arrays #20256

wagenet commented Nov 7, 2022 •

edited

Loading

chriskrycho commented Nov 7, 2022

chriskrycho Nov 7, 2022

chriskrycho Nov 9, 2022

wagenet Nov 7, 2022

wagenet Nov 7, 2022

wagenet Nov 7, 2022

wagenet Nov 7, 2022

wagenet Nov 7, 2022

wagenet Nov 7, 2022

wagenet Nov 7, 2022

wagenet Nov 7, 2022

wagenet Nov 7, 2022

chriskrycho Nov 9, 2022

wagenet Nov 9, 2022

chriskrycho Nov 9, 2022

chriskrycho Nov 9, 2022

wagenet Nov 9, 2022

chriskrycho left a comment

chriskrycho Nov 9, 2022

chriskrycho Nov 9, 2022

chriskrycho Nov 9, 2022

		let namespaces = Namespace.NAMESPACES;
		let types: string[] = [];

[BUGFIX release] Correct types for Ember Arrays #20256

[BUGFIX release] Correct types for Ember Arrays #20256

Conversation

wagenet commented Nov 7, 2022 • edited Loading

chriskrycho commented Nov 7, 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

chriskrycho left a 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

wagenet commented Nov 7, 2022 •

edited

Loading