From 9e2355b09c1993db6a332a1c1ebbc8ad3ef92317 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Sat, 13 Jan 2024 12:22:58 -0500 Subject: [PATCH] Refactor the map util to not be a resource, it never needed to be (#54) * Refactor the map util to not be a resource, it never needed to be * boop * Fix lint * Update test's typse to be less restructive --- .github/workflows/ci.yml | 2 + reactiveweb/src/map.ts | 119 +++++++-------- tests/test-app/tests/utils/map/js-test.ts | 175 ++++++++++++---------- 3 files changed, 157 insertions(+), 139 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ecc0931..e904019 100644 --- a/.github/workflows/ci.yml +++ b/.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: diff --git a/reactiveweb/src/map.ts b/reactiveweb/src/map.ts index d44ca11..fe6fad9 100644 --- a/reactiveweb/src/map.ts +++ b/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 = ExpandArgs['Positional']; -type Named = ExpandArgs['Named']; - /** - * Public API of the return value of the [[map]] resource. + * Public API of the return value of the [[map]] utility. */ export interface MappedArray { /** @@ -130,7 +124,7 @@ export interface MappedArray { * 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( * - 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 { let { data, map } = options; - // parens required, else ESLint and TypeScript/Glint error here - // prettier-ignore - let resource = (TrackedArrayMap).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 & { [K in keyof Elements]: MapTo }; + return new TrackedArrayMap(destroyable, data, map) as MappedArray; } -type Args = { - Positional: [E[] | readonly E[]]; - Named: { - map: (element: E) => Result; - }; -}; - -const AT = Symbol('__AT__'); +const AT = '__AT__'; /** * @private */ export class TrackedArrayMap - extends Resource> implements MappedArray { // Tells TS that we can array-index-access [index: number]: MappedTo; - #map = new WeakMap(); + // these can't be real private fields + // until @cached is a real decorator + private _mapCache = new WeakMap(); + 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]; + }, + // Is there a way to do this without lying to TypeScript? + }) as TrackedArrayMap; + } + + @cached + get _records(): (Element & object)[] { + let data = this._dataFn(); - modify([data]: Positional>, { map }: Named>) { assert( `Every entry in the data passed to \`map\` must be an object.`, data.every((datum) => typeof datum === 'object') ); - this._records = data as Array; - this._map = map; + + return data as Array; } values = () => [...this]; @@ -281,7 +276,7 @@ export class TrackedArrayMap * 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 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; - } + }; } diff --git a/tests/test-app/tests/utils/map/js-test.ts b/tests/test-app/tests/utils/map/js-test.ts index 0519d57..29bde0c 100644 --- a/tests/test-app/tests/utils/map/js-test.ts +++ b/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'); + }); + } });