Refactor the map util to not be a resource, it never needed to be

.github/workflows/ci.yml

@@ -34,6 +34,8 @@ jobs:
     steps:
       - uses: actions/checkout@v4
       - uses: wyvox/action-setup-pnpm@v3
+      - run: pnpm build
+      - run: pnpm i -f # re sync injected deps
       - run: pnpm lint
 
   test:

reactiveweb/src/map.ts

@@ -1,15 +1,9 @@
-import { tracked } from '@glimmer/tracking';
+import { cached } from '@glimmer/tracking';
+import { setOwner } from '@ember/application';
 import { assert } from '@ember/debug';
 
-import { Resource } from 'ember-resources';
-
-import type { ExpandArgs } from 'ember-resources';
-
-type Positional<T> = ExpandArgs<T>['Positional'];
-type Named<T> = ExpandArgs<T>['Named'];
-
 /**
- * Public API of the return value of the [[map]] resource.
+ * Public API of the return value of the [[map]] utility.
  */
 export interface MappedArray<Elements extends readonly unknown[], MappedTo> {
   /**
@@ -130,7 +124,7 @@ export interface MappedArray<Elements extends readonly unknown[], MappedTo> {
  * need to be transformed into a different shape (adding/removing/modifying data/properties)
  * and you want the transform to be efficient when iterating over that data.
  *
- * A common use case where this `map` resource provides benefits over is
+ * A common use case where this `map` utility provides benefits over is
  * ```js
  * class MyClass {\
  *   @cached
@@ -178,74 +172,75 @@ export function map<Elements extends readonly unknown[], MapTo = unknown>(
      * - if iterating over part of the data, map will only be called for the elements observed
      * - if not iterating, map will only be called for the elements observed.
      */
-    map: (element: Elements[number]) => MapTo;
+    map: (element: Elements[0]) => MapTo;
   }
-) {
+): MappedArray<Elements, MapTo> {
   let { data, map } = options;
 
-  // parens required, else ESLint and TypeScript/Glint error here
-  // prettier-ignore
-  let resource = (TrackedArrayMap<Elements[number], MapTo>).from(destroyable, () => {
-    let reified = data();
-
-    return { positional: [reified], named: { map } };
-  });
-
-  /**
-   * This is what allows square-bracket index-access to work.
-   *
-   * Unfortunately this means the returned value is
-   * Proxy -> Proxy -> wrapper object -> *then* the class instance
-   *
-   * Maybe JS has a way to implement array-index access, but I don't know how
-   */
-  return new Proxy(resource, {
-    get(target, property, receiver) {
-      if (typeof property === 'string') {
-        let parsed = parseInt(property, 10);
-
-        if (!isNaN(parsed)) {
-          return target[AT](parsed);
-        }
-      }
-
-      return Reflect.get(target, property, receiver);
-    },
-    // Is there a way to do this without lying to TypeScript?
-  }) as unknown as MappedArray<Elements, MapTo> & { [K in keyof Elements]: MapTo };
+  return new TrackedArrayMap(destroyable, data, map) as MappedArray<Elements, MapTo>;
 }
 
-type Args<E = unknown, Result = unknown> = {
-  Positional: [E[] | readonly E[]];
-  Named: {
-    map: (element: E) => Result;
-  };
-};
-
-const AT = Symbol('__AT__');
+const AT = '__AT__';
 
 /**
  * @private
  */
 export class TrackedArrayMap<Element = unknown, MappedTo = unknown>
-  extends Resource<Args<Element, MappedTo>>
   implements MappedArray<Element[], MappedTo>
 {
   // Tells TS that we can array-index-access
   [index: number]: MappedTo;
 
-  #map = new WeakMap<Element & object, MappedTo>();
+  // these can't be real private fields
+  // until @cached is a real decorator
+  private _mapCache = new WeakMap<Element & object, MappedTo>();
+  private _dataFn: () => readonly Element[];
+  private _mapper: (element: Element) => MappedTo;
+
+  constructor(owner: object, data: () => readonly Element[], map: (element: Element) => MappedTo) {
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    setOwner(this, owner as any);
 
-  @tracked private declare _records: (Element & object)[];
-  @tracked private declare _map: (element: Element) => MappedTo;
+    this._dataFn = data;
+    this._mapper = map;
+
+    // eslint-disable-next-line @typescript-eslint/no-this-alias
+    const self = this;
+
+    /**
+     * This is what allows square-bracket index-access to work.
+     *
+     * Unfortunately this means the returned value is
+     * Proxy -> Proxy -> wrapper object -> *then* the class instance
+     *
+     * Maybe JS has a way to implement array-index access, but I don't know how
+     */
+    return new Proxy(this, {
+      get(_target, property) {
+        if (typeof property === 'string') {
+          let parsed = parseInt(property, 10);
+
+          if (!isNaN(parsed)) {
+            return self[AT](parsed);
+          }
+        }
+
+        return self[property as keyof MappedArray<Element[], MappedTo>];
+      },
+      // Is there a way to do this without lying to TypeScript?
+    }) as TrackedArrayMap<Element, MappedTo>;
+  }
+
+  @cached
+  get _records(): (Element & object)[] {
+    let data = this._dataFn();
 
-  modify([data]: Positional<Args<Element, MappedTo>>, { map }: Named<Args<Element, MappedTo>>) {
     assert(
       `Every entry in the data passed to \`map\` must be an object.`,
       data.every((datum) => typeof datum === 'object')
     );
-    this._records = data as Array<Element & object>;
-    this._map = map;
+
+    return data as Array<Element & object>;
   }
 
   values = () => [...this];
@@ -281,7 +276,7 @@ export class TrackedArrayMap<Element = unknown, MappedTo = unknown>
    * don't conflict with
    *   https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/at
    */
-  [AT](i: number) {
+  [AT] = (i: number) => {
     let record = this._records[i];
 
     assert(
@@ -292,13 +287,13 @@ export class TrackedArrayMap<Element = unknown, MappedTo = unknown>
       record
     );
 
-    let value = this.#map.get(record);
+    let value = this._mapCache.get(record);
 
     if (!value) {
-      value = this._map(record);
-      this.#map.set(record, value);
+      value = this._mapper(record);
+      this._mapCache.set(record, value);
     }
 
     return value;
-  }
+  };
 }

tests/test-app/tests/utils/map/js-test.ts

@@ -30,81 +30,102 @@ module('Utils | map | js', function (hooks) {
     return new ExampleTrackedThing(id);
   }
 
-  test('it works', async function (assert) {
-    class Test {
-      @tracked records: TestRecord[] = [];
-
-      stuff = map(this, {
-        data: () => {
-          assert.step('evaluate data thunk');
-
-          return this.records;
-        },
-        map: (record) => {
-          assert.step(`perform map on ${record.id}`);
-
-          return new Wrapper(record);
-        },
-      });
-    }
-
-    let currentStuff: Wrapper[] = [];
-    let instance = new Test();
-
-    setOwner(instance, this.owner);
-
-    assert.strictEqual(instance.stuff.length, 0);
-    assert.verifySteps(['evaluate data thunk']);
-
-    let first = testData(1);
-    let second = testData(2);
-
-    instance.records = [first, second];
-    assert.strictEqual(instance.stuff.length, 2, 'length adjusted');
-    assert.verifySteps(
-      ['evaluate data thunk'],
-      'we do not map yet because the data has not been accessed'
-    );
-
-    assert.ok(instance.stuff[0] instanceof Wrapper, 'access id:1');
-    assert.ok(instance.stuff[1] instanceof Wrapper, 'access id:2');
-    assert.verifySteps(['perform map on 1', 'perform map on 2']);
-
-    assert.ok(instance.stuff[0] instanceof Wrapper, 'access id:1');
-    assert.ok(instance.stuff[1] instanceof Wrapper, 'access id:2');
-    assert.verifySteps([], 're-access does not re-map');
-
-    // this tests the iterator
-    currentStuff = [...instance.stuff];
-    assert.ok(instance.stuff.values()[0] instanceof Wrapper, 'mappedRecords id:1');
-    assert.ok(instance.stuff.values()[1] instanceof Wrapper, 'mappedRecords id:2');
-
-    assert.strictEqual(currentStuff[0]?.record, first, 'object equality retained');
-    assert.strictEqual(currentStuff[1]?.record, second, 'object equality retained');
-
-    instance.records = [...instance.records, testData(3)];
-    assert.strictEqual(instance.stuff.length, 3, 'length adjusted');
-    assert.verifySteps(
-      ['evaluate data thunk'],
-      'we do not map on the new object yet because the data has not been accessed'
-    );
-
-    assert.ok(instance.stuff[0] instanceof Wrapper, 'access id:1');
-    assert.ok(instance.stuff[1] instanceof Wrapper, 'access id:2');
-    assert.ok(instance.stuff[2] instanceof Wrapper, 'access id:3');
-    assert.strictEqual(instance.stuff[0], currentStuff[0], 'original objects retained');
-    assert.strictEqual(instance.stuff[1], currentStuff[1], 'original objects retained');
-    assert.verifySteps(
-      ['perform map on 3'],
-      'only calls map once, even though the whole source data was re-created'
-    );
-
-    first.someValue = 'throwaway value';
-    assert.verifySteps(
-      [],
-      'data thunk is not ran, because the tracked data consumed in the thunk was not changed'
-    );
-    assert.strictEqual(instance.stuff[0], currentStuff[0], 'original objects retained');
-    assert.strictEqual(instance.stuff[1], currentStuff[1], 'original objects retained');
-  });
+  for (let variant of [
+    {
+      name: 'public api',
+      getItem: (collection: any, index: number) => collection[index],
+    },
+    {
+      name: 'private api (sanity check)',
+      getItem: (collection: any, index: number) => {
+        // const AT = Symbol.for('__AT__');
+        const AT = '__AT__';
+
+        return collection[AT](index);
+      },
+    },
+  ]) {
+    test(`it works (${variant.name})`, async function (assert) {
+      class Test {
+        @tracked records: TestRecord[] = [];
+
+        stuff = map(this, {
+          data: () => {
+            assert.step('evaluate data thunk');
+
+            return this.records;
+          },
+          map: (record) => {
+            assert.step(`perform map on ${record.id}`);
+
+            return new Wrapper(record);
+          },
+        });
+      }
+
+      let currentStuff: Wrapper[] = [];
+      let instance = new Test();
+
+      const get = (index: number) => variant.getItem(instance.stuff, index);
+
+      setOwner(instance, this.owner);
+
+      assert.strictEqual(instance.stuff.length, 0);
+      assert.verifySteps(['evaluate data thunk'], '❯❯ initially, the data fn is consumed');
+
+      let first = testData(1);
+      let second = testData(2);
+
+      instance.records = [first, second];
+      assert.strictEqual(instance.stuff.length, 2, 'length adjusted');
+      assert.verifySteps(
+        ['evaluate data thunk'],
+        '❯❯ we do not map yet because the data has not been accessed'
+      );
+
+      assert.ok(get(0) instanceof Wrapper, 'access id:1');
+      assert.ok(get(1) instanceof Wrapper, 'access id:2');
+      assert.verifySteps(
+        ['perform map on 1', 'perform map on 2'],
+        '❯❯ accessing indicies calls the mapper'
+      );
+
+      assert.ok(get(0) instanceof Wrapper, 'access id:1');
+      assert.ok(get(1) instanceof Wrapper, 'access id:2');
+      assert.verifySteps([], '❯❯ re-access is a no-op');
+
+      // this tests the iterator
+      currentStuff = [...instance.stuff];
+      assert.ok(instance.stuff.values()[0] instanceof Wrapper, 'mappedRecords id:1');
+      assert.ok(instance.stuff.values()[1] instanceof Wrapper, 'mappedRecords id:2');
+
+      assert.strictEqual(currentStuff[0]?.record, first, 'object equality retained');
+      assert.strictEqual(currentStuff[1]?.record, second, 'object equality retained');
+
+      instance.records = [...instance.records, testData(3)];
+      assert.strictEqual(instance.stuff.length, 3, 'length adjusted');
+      assert.verifySteps(
+        ['evaluate data thunk'],
+        '❯❯ we do not map on the new object yet because the data has not been accessed'
+      );
+
+      assert.ok(get(0) instanceof Wrapper, 'access id:1');
+      assert.ok(get(1) instanceof Wrapper, 'access id:2');
+      assert.ok(get(2) instanceof Wrapper, 'access id:3');
+      assert.strictEqual(get(0), currentStuff[0], 'original objects retained');
+      assert.strictEqual(get(1), currentStuff[1], 'original objects retained');
+      assert.verifySteps(
+        ['perform map on 3'],
+        '❯❯ only calls map once, even though the whole source data was re-created'
+      );
+
+      first.someValue = 'throwaway value';
+      assert.verifySteps(
+        [],
+        '❯❯ data thunk is not ran, because the tracked data consumed in the thunk was not changed'
+      );
+      assert.strictEqual(get(0), currentStuff[0], 'original objects retained');
+      assert.strictEqual(get(1), currentStuff[1], 'original objects retained');
+    });
+  }
 });