diff --git a/packages/model/src/-private/many-array.ts b/packages/model/src/-private/many-array.ts index 0b57eb26057..198715f1ce1 100644 --- a/packages/model/src/-private/many-array.ts +++ b/packages/model/src/-private/many-array.ts @@ -1,11 +1,13 @@ /** @module @ember-data/store */ -import { assert } from '@ember/debug'; +import { assert, deprecate } from '@ember/debug'; +import { DEPRECATE_MANY_ARRAY_DUPLICATES } from '@ember-data/deprecations'; import type Store from '@ember-data/store'; import { ARRAY_SIGNAL, + isStableIdentifier, MUTATE, notifyArray, RecordArray, @@ -18,6 +20,8 @@ import type { Cache } from '@ember-data/store/-types/q/cache'; import type { ModelSchema } from '@ember-data/store/-types/q/ds-model'; import type { RecordInstance } from '@ember-data/store/-types/q/record-instance'; import type { FindOptions } from '@ember-data/store/-types/q/store'; +import type { Signal } from '@ember-data/tracking/-private'; +import { addToTransaction } from '@ember-data/tracking/-private'; import type { StableRecordIdentifier } from '@warp-drive/core-types'; import type { Links, PaginationLinks } from '@warp-drive/core-types/spec/raw'; @@ -163,111 +167,210 @@ export default class RelatedCollection extends RecordArray { this.key = options.key; } - [MUTATE](prop: string, args: unknown[], result?: unknown) { + [MUTATE]( + target: StableRecordIdentifier[], + receiver: typeof Proxy, + prop: string, + args: unknown[], + _SIGNAL: Signal + ): unknown { switch (prop) { case 'length 0': { - this._manager.mutate({ - op: 'replaceRelatedRecords', - record: this.identifier, - field: this.key, - value: [], - }); - break; + Reflect.set(target, 'length', 0); + mutateReplaceRelatedRecords(this, [], _SIGNAL); + return true; } case 'replace cell': { const [index, prior, value] = args as [number, StableRecordIdentifier, StableRecordIdentifier]; - this._manager.mutate({ - op: 'replaceRelatedRecord', - record: this.identifier, - field: this.key, - value, - prior, - index, - }); - break; + target[index] = value; + mutateReplaceRelatedRecord(this, { value, prior, index }, _SIGNAL); + return true; } - case 'push': - this._manager.mutate({ - op: 'addToRelatedRecords', - record: this.identifier, - field: this.key, - value: extractIdentifiersFromRecords(args), - }); - break; - case 'pop': - if (result) { - this._manager.mutate({ - op: 'removeFromRelatedRecords', - record: this.identifier, - field: this.key, - value: recordIdentifierFor(result as RecordInstance), + case 'push': { + const newValues = extractIdentifiersFromRecords(args); + + assertNoDuplicates( + this, + target, + (currentState) => currentState.push(...newValues), + `Cannot push duplicates to a hasMany's state.` + ); + + if (DEPRECATE_MANY_ARRAY_DUPLICATES) { + // dedupe + const seen = new Set(target); + const unique = new Set(); + + args.forEach((item) => { + const identifier = recordIdentifierFor(item); + if (!seen.has(identifier)) { + seen.add(identifier); + unique.add(item); + } }); + + const newArgs = Array.from(unique); + const result = Reflect.apply(target[prop], receiver, newArgs) as RecordInstance[]; + + if (newArgs.length) { + mutateAddToRelatedRecords(this, { value: extractIdentifiersFromRecords(newArgs) }, _SIGNAL); + } + return result; + } + + // else, no dedupe, error on duplicates + const result = Reflect.apply(target[prop], receiver, args) as RecordInstance[]; + if (newValues.length) { + mutateAddToRelatedRecords(this, { value: newValues }, _SIGNAL); } - break; - - case 'unshift': - this._manager.mutate({ - op: 'addToRelatedRecords', - record: this.identifier, - field: this.key, - value: extractIdentifiersFromRecords(args), - index: 0, - }); - break; - - case 'shift': + return result; + } + + case 'pop': { + const result: unknown = Reflect.apply(target[prop], receiver, args); if (result) { - this._manager.mutate({ - op: 'removeFromRelatedRecords', - record: this.identifier, - field: this.key, - value: recordIdentifierFor(result as RecordInstance), - index: 0, + mutateRemoveFromRelatedRecords(this, { value: recordIdentifierFor(result as RecordInstance) }, _SIGNAL); + } + return result; + } + + case 'unshift': { + const newValues = extractIdentifiersFromRecords(args); + + assertNoDuplicates( + this, + target, + (currentState) => currentState.unshift(...newValues), + `Cannot unshift duplicates to a hasMany's state.` + ); + + if (DEPRECATE_MANY_ARRAY_DUPLICATES) { + // dedupe + const seen = new Set(target); + const unique = new Set(); + + args.forEach((item) => { + const identifier = recordIdentifierFor(item); + if (!seen.has(identifier)) { + seen.add(identifier); + unique.add(item); + } }); + + const newArgs = Array.from(unique); + const result: unknown = Reflect.apply(target[prop], receiver, newArgs); + + if (newArgs.length) { + mutateAddToRelatedRecords(this, { value: extractIdentifiersFromRecords(newArgs), index: 0 }, _SIGNAL); + } + return result; } - break; - case 'sort': - this._manager.mutate({ - op: 'sortRelatedRecords', - record: this.identifier, - field: this.key, - value: (result as RecordInstance[]).map(recordIdentifierFor), - }); - break; + // else, no dedupe, error on duplicates + const result = Reflect.apply(target[prop], receiver, args) as RecordInstance[]; + if (newValues.length) { + mutateAddToRelatedRecords(this, { value: newValues, index: 0 }, _SIGNAL); + } + return result; + } + + case 'shift': { + const result: unknown = Reflect.apply(target[prop], receiver, args); + + if (result) { + mutateRemoveFromRelatedRecords( + this, + { value: recordIdentifierFor(result as RecordInstance), index: 0 }, + _SIGNAL + ); + } + return result; + } + + case 'sort': { + const result: unknown = Reflect.apply(target[prop], receiver, args); + mutateSortRelatedRecords(this, (result as RecordInstance[]).map(recordIdentifierFor), _SIGNAL); + return result; + } case 'splice': { - const [start, removeCount, ...adds] = args as [number, number, RecordInstance]; + const [start, deleteCount, ...adds] = args as [number, number, ...RecordInstance[]]; + // detect a full replace - if (removeCount > 0 && adds.length === this[SOURCE].length) { - this._manager.mutate({ - op: 'replaceRelatedRecords', - record: this.identifier, - field: this.key, - value: extractIdentifiersFromRecords(adds), - }); - return; - } - if (removeCount > 0) { - this._manager.mutate({ - op: 'removeFromRelatedRecords', - record: this.identifier, - field: this.key, - value: (result as RecordInstance[]).map(recordIdentifierFor), - index: start, - }); + if (start === 0 && deleteCount === this[SOURCE].length) { + const newValues = extractIdentifiersFromRecords(adds); + + assertNoDuplicates( + this, + target, + (currentState) => currentState.splice(start, deleteCount, ...newValues), + `Cannot replace a hasMany's state with a new state that contains duplicates.` + ); + + if (DEPRECATE_MANY_ARRAY_DUPLICATES) { + // dedupe + const current = new Set(adds); + const unique = Array.from(current); + const newArgs = ([start, deleteCount] as unknown[]).concat(unique); + + const result = Reflect.apply(target[prop], receiver, newArgs) as RecordInstance[]; + + mutateReplaceRelatedRecords(this, extractIdentifiersFromRecords(unique), _SIGNAL); + return result; + } + + // else, no dedupe, error on duplicates + const result = Reflect.apply(target[prop], receiver, args) as RecordInstance[]; + mutateReplaceRelatedRecords(this, newValues, _SIGNAL); + return result; } - if (adds?.length) { - this._manager.mutate({ - op: 'addToRelatedRecords', - record: this.identifier, - field: this.key, - value: extractIdentifiersFromRecords(adds), - index: start, + + const newValues = extractIdentifiersFromRecords(adds); + assertNoDuplicates( + this, + target, + (currentState) => currentState.splice(start, deleteCount, ...newValues), + `Cannot splice a hasMany's state with a new state that contains duplicates.` + ); + + if (DEPRECATE_MANY_ARRAY_DUPLICATES) { + // dedupe + const currentState = target.slice(); + currentState.splice(start, deleteCount); + + const seen = new Set(currentState); + const unique: RecordInstance[] = []; + adds.forEach((item) => { + const identifier = recordIdentifierFor(item); + if (!seen.has(identifier)) { + seen.add(identifier); + unique.push(item); + } }); + + const newArgs = [start, deleteCount, ...unique]; + const result = Reflect.apply(target[prop], receiver, newArgs) as RecordInstance[]; + + if (deleteCount > 0) { + mutateRemoveFromRelatedRecords(this, { value: result.map(recordIdentifierFor), index: start }, _SIGNAL); + } + + if (unique.length > 0) { + mutateAddToRelatedRecords(this, { value: extractIdentifiersFromRecords(unique), index: start }, _SIGNAL); + } + + return result; } - break; + // else, no dedupe, error on duplicates + const result = Reflect.apply(target[prop], receiver, args) as RecordInstance[]; + if (deleteCount > 0) { + mutateRemoveFromRelatedRecords(this, { value: result.map(recordIdentifierFor), index: start }, _SIGNAL); + } + if (newValues.length > 0) { + mutateAddToRelatedRecords(this, { value: newValues, index: start }, _SIGNAL); + } + return result; } default: assert(`unable to convert ${prop} into a transaction that updates the cache state for this record array`); @@ -380,3 +483,137 @@ function extractIdentifierFromRecord(recordOrPromiseRecord: PromiseProxyRecord | assertRecordPassedToHasMany(recordOrPromiseRecord); return recordIdentifierFor(recordOrPromiseRecord); } + +function assertNoDuplicates( + collection: RelatedCollection, + target: StableRecordIdentifier[], + callback: (currentState: StableRecordIdentifier[]) => void, + reason: string +) { + const state = target.slice(); + callback(state); + + if (state.length !== new Set(state).size) { + const duplicates = state.filter((currentValue, currentIndex) => state.indexOf(currentValue) !== currentIndex); + + if (DEPRECATE_MANY_ARRAY_DUPLICATES) { + deprecate( + `${reason} This behavior is deprecated. Found duplicates for the following records within the new state provided to \`<${ + collection.identifier.type + }:${collection.identifier.id || collection.identifier.lid}>.${collection.key}\`\n\t- ${Array.from( + new Set(duplicates) + ) + .map((r) => (isStableIdentifier(r) ? r.lid : recordIdentifierFor(r).lid)) + .sort((a, b) => a.localeCompare(b)) + .join('\n\t- ')}`, + false, + { + id: 'ember-data:deprecate-many-array-duplicates', + for: 'ember-data', + until: '6.0', + since: { + enabled: '5.3', + available: '5.3', + }, + } + ); + } else { + throw new Error( + `${reason} Found duplicates for the following records within the new state provided to \`<${ + collection.identifier.type + }:${collection.identifier.id || collection.identifier.lid}>.${collection.key}\`\n\t- ${Array.from( + new Set(duplicates) + ) + .map((r) => (isStableIdentifier(r) ? r.lid : recordIdentifierFor(r).lid)) + .sort((a, b) => a.localeCompare(b)) + .join('\n\t- ')}` + ); + } + } +} + +function mutateAddToRelatedRecords( + collection: RelatedCollection, + operationInfo: { value: StableRecordIdentifier | StableRecordIdentifier[]; index?: number }, + _SIGNAL: Signal +) { + mutate( + collection, + { + op: 'addToRelatedRecords', + record: collection.identifier, + field: collection.key, + ...operationInfo, + }, + _SIGNAL + ); +} + +function mutateRemoveFromRelatedRecords( + collection: RelatedCollection, + operationInfo: { value: StableRecordIdentifier | StableRecordIdentifier[]; index?: number }, + _SIGNAL: Signal +) { + mutate( + collection, + { + op: 'removeFromRelatedRecords', + record: collection.identifier, + field: collection.key, + ...operationInfo, + }, + _SIGNAL + ); +} + +function mutateReplaceRelatedRecord( + collection: RelatedCollection, + operationInfo: { + value: StableRecordIdentifier; + prior: StableRecordIdentifier; + index: number; + }, + _SIGNAL: Signal +) { + mutate( + collection, + { + op: 'replaceRelatedRecord', + record: collection.identifier, + field: collection.key, + ...operationInfo, + }, + _SIGNAL + ); +} + +function mutateReplaceRelatedRecords(collection: RelatedCollection, value: StableRecordIdentifier[], _SIGNAL: Signal) { + mutate( + collection, + { + op: 'replaceRelatedRecords', + record: collection.identifier, + field: collection.key, + value, + }, + _SIGNAL + ); +} + +function mutateSortRelatedRecords(collection: RelatedCollection, value: StableRecordIdentifier[], _SIGNAL: Signal) { + mutate( + collection, + { + op: 'sortRelatedRecords', + record: collection.identifier, + field: collection.key, + value, + }, + _SIGNAL + ); +} + +function mutate(collection: RelatedCollection, mutation: Parameters[0], _SIGNAL: Signal) { + collection._manager.mutate(mutation); + addToTransaction(_SIGNAL); +} diff --git a/packages/private-build-infra/virtual-packages/deprecations.d.ts b/packages/private-build-infra/virtual-packages/deprecations.d.ts index 5cc95073662..064b4cef983 100644 --- a/packages/private-build-infra/virtual-packages/deprecations.d.ts +++ b/packages/private-build-infra/virtual-packages/deprecations.d.ts @@ -6,3 +6,4 @@ export const DEPRECATE_NON_STRICT_ID: boolean; export const DEPRECATE_LEGACY_IMPORTS: boolean; export const DEPRECATE_NON_UNIQUE_PAYLOADS: boolean; export const DEPRECATE_RELATIONSHIP_REMOTE_UPDATE_CLEARING_LOCAL_STATE: boolean; +export const DEPRECATE_MANY_ARRAY_DUPLICATES: boolean; diff --git a/packages/private-build-infra/virtual-packages/deprecations.js b/packages/private-build-infra/virtual-packages/deprecations.js index b1ce39c4208..5f2256cea3d 100644 --- a/packages/private-build-infra/virtual-packages/deprecations.js +++ b/packages/private-build-infra/virtual-packages/deprecations.js @@ -369,3 +369,20 @@ export const DEPRECATE_NON_UNIQUE_PAYLOADS = '5.3'; * @public */ export const DEPRECATE_RELATIONSHIP_REMOTE_UPDATE_CLEARING_LOCAL_STATE = '5.3'; + +/** + * **id: ember-data:deprecate-many-array-duplicates** + * + * When the flag is `true` (default), adding duplicate records to a `ManyArray` + * is deprecated in non-production environments. In production environments, + * duplicate records added to a `ManyArray` will be deduped and no error will + * be thrown. + * + * When the flag is `false`, an error will be thrown when duplicates are added. + * + * @property DEPRECATE_MANY_ARRAY_DUPLICATES + * @since 5.3 + * @until 6.0 + * @public + */ +export const DEPRECATE_MANY_ARRAY_DUPLICATES = '5.3'; diff --git a/packages/store/src/-private/managers/cache-capabilities-manager.ts b/packages/store/src/-private/managers/cache-capabilities-manager.ts index afaa8148196..74743f46881 100644 --- a/packages/store/src/-private/managers/cache-capabilities-manager.ts +++ b/packages/store/src/-private/managers/cache-capabilities-manager.ts @@ -47,6 +47,8 @@ export class CacheCapabilitiesManager implements StoreWrapper { if (this._store._cbs) { this._store._schedule('notify', () => this._flushNotifications()); } else { + // TODO @runspired determine if relationship mutations should schedule + // into join/run vs immediate flush this._flushNotifications(); } } diff --git a/packages/store/src/-private/managers/cache-manager.ts b/packages/store/src/-private/managers/cache-manager.ts index 4328696d75f..3ae5f5454ca 100644 --- a/packages/store/src/-private/managers/cache-manager.ts +++ b/packages/store/src/-private/managers/cache-manager.ts @@ -49,7 +49,7 @@ export class CacheManager implements Cache { * semantics, `put` has `replace` semantics similar to * the `http` method `PUT` * - * the individually cacheabl + * the individually cacheable * e resource data it may contain * should upsert, but the document data surrounding it should * fully replace any existing information @@ -113,7 +113,7 @@ export class CacheManager implements Cache { * An implementation might want to do this because * de-referencing records which read from their own * blob is generally safer because the record does - * not require retainining connections to the Store + * not require retaining connections to the Store * and Cache to present data on a per-field basis. * * This generally takes the place of `getAttr` as @@ -276,7 +276,7 @@ export class CacheManager implements Cache { // ================ /** - * [LIFECYLCE] Signal to the cache that a new record has been instantiated on the client + * [LIFECYCLE] Signal to the cache that a new record has been instantiated on the client * * It returns properties from options that should be set on the record during the create * process. This return value behavior is deprecated. diff --git a/packages/store/src/-private/record-arrays/identifier-array.ts b/packages/store/src/-private/record-arrays/identifier-array.ts index 58a1c25f9ef..2642aafd8de 100644 --- a/packages/store/src/-private/record-arrays/identifier-array.ts +++ b/packages/store/src/-private/record-arrays/identifier-array.ts @@ -17,6 +17,7 @@ import type { ImmutableRequestInfo } from '@warp-drive/core-types/request'; import type { Links, PaginationLinks } from '@warp-drive/core-types/spec/raw'; import type { RecordInstance } from '../../-types/q/record-instance'; +import { isStableIdentifier } from '../caches/identifier-cache'; import { recordIdentifierFor } from '../caches/instance-cache'; import type RecordArrayManager from '../managers/record-array-manager'; import type Store from '../store-service'; @@ -99,9 +100,9 @@ interface PrivateState { links: Links | PaginationLinks | null; meta: Record | null; } -type ForEachCB = (record: RecordInstance, index: number, context: IdentifierArray) => void; +type ForEachCB = (record: RecordInstance, index: number, context: typeof Proxy) => void; function safeForEach( - instance: IdentifierArray, + instance: typeof Proxy, arr: StableRecordIdentifier[], store: Store, callback: ForEachCB, @@ -140,7 +141,13 @@ function safeForEach( @public */ interface IdentifierArray extends Omit, '[]'> { - [MUTATE]?(prop: string, args: unknown[], result?: unknown): void; + [MUTATE]?( + target: StableRecordIdentifier[], + receiver: typeof Proxy, + prop: string, + args: unknown[], + _SIGNAL: Signal + ): unknown; } class IdentifierArray { declare DEPRECATED_CLASS_NAME: string; @@ -223,10 +230,14 @@ class IdentifierArray { // and forward them as one const proxy = new Proxy(this[SOURCE], { - get(target: StableRecordIdentifier[], prop: keyof R, receiver: R): unknown { + get>( + target: StableRecordIdentifier[], + prop: keyof R, + receiver: R + ): unknown { const index = convertToInt(prop); if (_SIGNAL.shouldReset && (index !== null || SYNC_PROPS.has(prop) || isArrayGetter(prop))) { - options.manager._syncArray(receiver); + options.manager._syncArray(receiver as unknown as IdentifierArray); _SIGNAL.t = false; _SIGNAL.shouldReset = false; } @@ -287,10 +298,7 @@ class IdentifierArray { const args: unknown[] = Array.prototype.slice.call(arguments); assert(`Cannot start a new array transaction while a previous transaction is underway`, !transaction); transaction = true; - const result: unknown = Reflect.apply(target[prop] as ProxiedMethod, receiver, args); - self[MUTATE]!(prop as string, args, result); - addToTransaction(_SIGNAL); - // TODO handle cache updates + const result = self[MUTATE]!(target, receiver, prop as string, args, _SIGNAL); transaction = false; return result; }; @@ -329,13 +337,17 @@ class IdentifierArray { return target[prop as keyof StableRecordIdentifier[]]; }, - set(target: StableRecordIdentifier[], prop: KeyType, value: unknown /*, receiver */): boolean { + // FIXME: Should this get a generic like get above? + set( + target: StableRecordIdentifier[], + prop: KeyType, + value: unknown, + receiver: typeof Proxy + ): boolean { if (prop === 'length') { if (!transaction && value === 0) { transaction = true; - addToTransaction(_SIGNAL); - Reflect.set(target, prop, value); - self[MUTATE]!('length 0', []); + self[MUTATE]!(target, receiver, 'length 0', [], _SIGNAL); transaction = false; return true; } else if (transaction) { @@ -354,8 +366,20 @@ class IdentifierArray { } const index = convertToInt(prop); + // we do not allow "holey" arrays and so if the index is + // greater than length then we will disallow setting it. + // however, there is a special case for "unshift" with more than + // one item being inserted since current items will be moved to the + // new indices first. + // we "loosely" detect this by just checking whether we are in + // a transaction. if (index === null || index > target.length) { - if (isSelfProp(self, prop)) { + if (index !== null && transaction) { + const identifier = recordIdentifierFor(value); + assert(`Cannot set index ${index} past the end of the array.`, isStableIdentifier(identifier)); + target[index] = identifier; + return true; + } else if (isSelfProp(self, prop)) { self[prop] = value; return true; } @@ -370,9 +394,27 @@ class IdentifierArray { const original: StableRecordIdentifier | undefined = target[index]; const newIdentifier = extractIdentifierFromRecord(value); (target as unknown as Record)[index] = newIdentifier; + assert(`Expected a record`, isStableIdentifier(newIdentifier)); + // We generate "transactions" whenever a setter method on the array + // is called and might bulk update multiple array cells. Fundamentally, + // all array operations decompose into individual cell replacements. + // e.g. a push is really a "replace cell at next index with new value" + // or a splice is "shift all values left/right by X and set out of new + // bounds cells to undefined" + // + // so, if we are in a transaction, then this is not a user generated change + // but one generated by a setter method. In this case we want to only apply + // the change to the target array and not call the MUTATE method. + // If there is no transaction though, then this means the user themselves has + // directly changed the value of a specific index and we need to thus generate + // a mutation for that change. + // e.g. "arr.push(newVal)" is handled by a "addToRelatedRecords" mutation within + // a transaction. + // while "arr[arr.length] = newVal;" is handled by this replace cell code path. if (!transaction) { - self[MUTATE]!('replace cell', [index, original, newIdentifier]); - addToTransaction(_SIGNAL); + self[MUTATE]!(target, receiver, 'replace cell', [index, original, newIdentifier], _SIGNAL); + } else { + target[index] = newIdentifier; } return true; @@ -475,7 +517,7 @@ class IdentifierArray { // this will error if someone tries to call // A(identifierArray) since it is not configurable -// which is preferrable to the `meta` override we used +// which is preferable to the `meta` override we used // before which required importing all of Ember const desc = { enumerable: true, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 3d8b3a9ba4f..55e1c1f4af9 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -4214,6 +4214,9 @@ importers: '@babel/runtime': specifier: ^7.23.6 version: 7.23.6 + '@ember-data/debug': + specifier: workspace:5.5.0-alpha.11 + version: file:packages/debug(@ember-data/store@5.5.0-alpha.11)(@ember/string@3.1.1) '@ember-data/graph': specifier: workspace:5.5.0-alpha.11 version: file:packages/graph(@babel/core@7.23.6)(@ember-data/store@5.5.0-alpha.11)(@warp-drive/core-types@5.5.0-alpha.11) @@ -4341,6 +4344,8 @@ importers: specifier: ^5.89.0 version: 5.89.0 dependenciesMeta: + '@ember-data/debug': + injected: true '@ember-data/graph': injected: true '@ember-data/json-api': diff --git a/tests/docs/fixtures/expected.js b/tests/docs/fixtures/expected.js index 0ebee8e31fa..37fff913905 100644 --- a/tests/docs/fixtures/expected.js +++ b/tests/docs/fixtures/expected.js @@ -71,6 +71,7 @@ module.exports = { '(private) @ember-data/debug InspectorDataAdapter#watchTypeIfUnseen', '(public) @ember-data/deprecations CurrentDeprecations#DEPRECATE_COMPUTED_CHAINS', '(public) @ember-data/deprecations CurrentDeprecations#DEPRECATE_LEGACY_IMPORTS', + '(public) @ember-data/deprecations CurrentDeprecations#DEPRECATE_MANY_ARRAY_DUPLICATES', '(public) @ember-data/deprecations CurrentDeprecations#DEPRECATE_NON_STRICT_ID', '(public) @ember-data/deprecations CurrentDeprecations#DEPRECATE_NON_STRICT_TYPES', '(public) @ember-data/deprecations CurrentDeprecations#DEPRECATE_NON_UNIQUE_PAYLOADS', diff --git a/tests/ember-data__graph/tests/integration/graph/edge-removal/helpers.ts b/tests/ember-data__graph/tests/integration/graph/edge-removal/helpers.ts index 0cdeebd9f69..3d46db9265e 100644 --- a/tests/ember-data__graph/tests/integration/graph/edge-removal/helpers.ts +++ b/tests/ember-data__graph/tests/integration/graph/edge-removal/helpers.ts @@ -144,8 +144,10 @@ export async function setInitialState(context: Context, config: TestConfig, asse if (isMany) { let friends: UserRecord[] = await (john.bestFriends as unknown as Promise); friends.push(chris); - friends = await (chris.bestFriends as unknown as Promise); - friends.push(john); + if (config.inverseNull) { + friends = await (chris.bestFriends as unknown as Promise); + friends.push(john); + } } else { // @ts-expect-error john.bestFriends = chris; diff --git a/tests/main/tests/helpers/reactive-context.ts b/tests/main/tests/helpers/reactive-context.ts new file mode 100644 index 00000000000..68d1acb3c9d --- /dev/null +++ b/tests/main/tests/helpers/reactive-context.ts @@ -0,0 +1,73 @@ +import type { TestContext } from '@ember/test-helpers'; +import { render } from '@ember/test-helpers'; +import Component from '@glimmer/component'; + +import { hbs } from 'ember-cli-htmlbars'; + +import type Model from '@ember-data/model'; + +export interface ReactiveContext { + counters: Record; + fieldOrder: string[]; + reset: () => void; +} + +export async function reactiveContext( + this: TestContext, + record: T, + fields: { name: string; type: 'field' | 'hasMany' | 'belongsTo' }[] +): Promise { + const _fields: string[] = []; + fields.forEach((field) => { + _fields.push(field.name + 'Count'); + _fields.push(field.name); + }); + + class ReactiveComponent extends Component { + get __allFields() { + return _fields; + } + } + const counters: Record = {}; + + fields.forEach((field) => { + counters[field.name] = 0; + Object.defineProperty(ReactiveComponent.prototype, field.name + 'Count', { + get() { + return counters[field.name]; + }, + }); + Object.defineProperty(ReactiveComponent.prototype, field.name, { + get() { + counters[field.name]++; + switch (field.type) { + case 'hasMany': + return `[${(record[field.name as keyof T] as Model[]).map((r) => r.id).join(',')}]`; + case 'belongsTo': + return (record[field.name as keyof T] as Model).id; + case 'field': + return record[field.name as keyof T] as unknown; + default: + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + throw new Error(`Unknown field type ${field.type} for field ${field.name}`); + } + }, + }); + }); + + this.owner.register('component:reactive-component', ReactiveComponent); + this.owner.register( + 'template:components/reactive-component', + hbs`
    {{#each this.__allFields as |prop|}}
  • {{prop}}: {{get this prop}}
    • {{/each}}
` + ); + + await render(hbs``); + + function reset() { + fields.forEach((field) => { + counters[field.name] = 0; + }); + } + + return { counters, reset, fieldOrder: _fields }; +} diff --git a/tests/main/tests/integration/relationships/collection/mutating-has-many-test.ts b/tests/main/tests/integration/relationships/collection/mutating-has-many-test.ts new file mode 100644 index 00000000000..3e19df94e5a --- /dev/null +++ b/tests/main/tests/integration/relationships/collection/mutating-has-many-test.ts @@ -0,0 +1,421 @@ +import { settled } from '@ember/test-helpers'; + +import { module, test } from 'qunit'; + +import { setupRenderingTest } from 'ember-qunit'; + +import { DEPRECATE_MANY_ARRAY_DUPLICATES } from '@ember-data/deprecations'; +import Model, { attr, hasMany } from '@ember-data/model'; +import type Store from '@ember-data/store'; +import { recordIdentifierFor } from '@ember-data/store'; +import type { ExistingResourceIdentifierObject } from '@warp-drive/core-types/spec/raw'; + +import type { ReactiveContext } from '../../../helpers/reactive-context'; +import { reactiveContext } from '../../../helpers/reactive-context'; + +let IS_DEPRECATE_MANY_ARRAY_DUPLICATES = false; + +if (DEPRECATE_MANY_ARRAY_DUPLICATES) { + IS_DEPRECATE_MANY_ARRAY_DUPLICATES = true; +} + +class User extends Model { + @attr declare name: string; + // eslint-disable-next-line @typescript-eslint/no-unsafe-call + @hasMany('user', { async: false, inverse: 'friends' }) declare friends: User[]; +} + +function krystanData() { + return { + id: '2', + type: 'user', + attributes: { + name: 'Krystan', + }, + }; +} + +function krystanRef(): ExistingResourceIdentifierObject { + return { type: 'user', id: '2' }; +} + +function samData() { + return { + id: '3', + type: 'user', + attributes: { + name: 'Sam', + }, + }; +} + +function samRef(): ExistingResourceIdentifierObject { + return { type: 'user', id: '3' }; +} + +function ericData() { + return { + id: '4', + type: 'user', + attributes: { + name: 'Eric', + }, + }; +} + +function ericRef(): ExistingResourceIdentifierObject { + return { type: 'user', id: '4' }; +} + +function chrisData(friends: ExistingResourceIdentifierObject[]) { + return { + id: '1', + type: 'user', + attributes: { + name: 'Chris', + }, + relationships: { + friends: { + data: friends, + }, + }, + }; +} + +function makeUser(store: Store, friends: ExistingResourceIdentifierObject[]): User { + return store.push({ + data: chrisData(friends), + included: [krystanData(), samData(), ericData()], + }) as User; +} + +type Mutation = { + name: string; + method: 'push' | 'unshift' | 'splice'; + values: ExistingResourceIdentifierObject[]; + start?: (record: User) => number; + deleteCount?: (record: User) => number; +}; + +function generateAppliedMutation(store: Store, record: User, mutation: Mutation) { + const friends = record.friends; + let outcomeValues: User[]; + let error: string; + + let seen = new Set(); + const duplicates = new Set(); + let outcome: User[]; + + switch (mutation.method) { + case 'push': + error = "Cannot push duplicates to a hasMany's state."; + outcomeValues = [...friends, ...mutation.values.map((ref) => store.peekRecord(ref) as User)]; + + outcomeValues.forEach((item) => { + if (seen.has(item)) { + duplicates.add(item); + } else { + seen.add(item); + } + }); + + outcome = Array.from(new Set(outcomeValues)); + + break; + case 'unshift': { + error = "Cannot unshift duplicates to a hasMany's state."; + const added = mutation.values.map((ref) => store.peekRecord(ref) as User); + seen = new Set(friends); + outcome = []; + added.forEach((item) => { + if (seen.has(item)) { + duplicates.add(item); + } else { + seen.add(item); + outcome.push(item); + } + }); + outcome.push(...friends); + break; + } + case 'splice': { + const start = mutation.start?.(record) ?? 0; + const deleteCount = mutation.deleteCount?.(record) ?? 0; + outcomeValues = friends.slice(); + const added = mutation.values.map((ref) => store.peekRecord(ref) as User); + outcomeValues.splice(start, deleteCount, ...added); + + if (start === 0 && deleteCount === friends.length) { + error = `Cannot replace a hasMany's state with a new state that contains duplicates.`; + + outcomeValues.forEach((item) => { + if (seen.has(item)) { + duplicates.add(item); + } else { + seen.add(item); + } + }); + + outcome = Array.from(new Set(outcomeValues)); + } else { + error = "Cannot splice a hasMany's state with a new state that contains duplicates."; + + const reducedFriends = friends.slice(); + reducedFriends.splice(start, deleteCount); + seen = new Set(reducedFriends); + const unique: User[] = []; + + added.forEach((item) => { + if (seen.has(item)) { + duplicates.add(item); + } else { + seen.add(item); + unique.push(item); + } + }); + reducedFriends.splice(start, 0, ...unique); + outcome = reducedFriends; + } + break; + } + } + + const hasDuplicates = duplicates.size > 0; + return { + hasDuplicates, + duplicates: Array.from(duplicates), + deduped: { + length: outcome.length, + membership: outcome, + ids: outcome.map((v) => v.id), + }, + unchanged: { + length: friends.length, + membership: friends.slice(), + ids: friends.map((v) => v.id), + }, + error, + }; +} + +async function applyMutation(assert: Assert, store: Store, record: User, mutation: Mutation, rc: ReactiveContext) { + assert.ok(true, `LOG: applying "${mutation.name}" with ids [${mutation.values.map((v) => v.id).join(',')}]`); + + const { counters, fieldOrder } = rc; + const friendsIndex = fieldOrder.indexOf('friends'); + const initialFriendsCount = counters.friends; + if (initialFriendsCount === undefined) { + throw new Error('could not find counters.friends'); + } + + const result = generateAppliedMutation(store, record, mutation); + const initialIds = record.friends.map((f) => f.id).join(','); + + const shouldError = result.hasDuplicates && !IS_DEPRECATE_MANY_ARRAY_DUPLICATES; + const shouldDeprecate = result.hasDuplicates && IS_DEPRECATE_MANY_ARRAY_DUPLICATES; + const expected = shouldError ? result.unchanged : result.deduped; + + try { + switch (mutation.method) { + case 'push': + record.friends.push(...mutation.values.map((ref) => store.peekRecord(ref) as User)); + break; + case 'unshift': + record.friends.unshift(...mutation.values.map((ref) => store.peekRecord(ref) as User)); + break; + case 'splice': + record.friends.splice( + mutation.start?.(record) ?? 0, + mutation.deleteCount?.(record) ?? 0, + ...mutation.values.map((ref) => store.peekRecord(ref) as User) + ); + break; + } + assert.ok(!shouldError, `expected error ${shouldError ? '' : 'NOT '}to be thrown`); + if (shouldDeprecate) { + const expectedMessage = `${ + result.error + } This behavior is deprecated. Found duplicates for the following records within the new state provided to \`.friends\`\n\t- ${Array.from(result.duplicates) + .map((r) => recordIdentifierFor(r).lid) + .sort((a, b) => a.localeCompare(b)) + .join('\n\t- ')}`; + assert.expectDeprecation({ + id: 'ember-data:deprecate-many-array-duplicates', + until: '6.0', + count: 1, + message: expectedMessage, + }); + } + } catch (e) { + assert.ok(shouldError, `expected error ${shouldError ? '' : 'NOT '}to be thrown`); + const expectedMessage = shouldError + ? `${result.error} Found duplicates for the following records within the new state provided to \`.friends\`\n\t- ${Array.from(result.duplicates) + .map((r) => recordIdentifierFor(r).lid) + .sort((a, b) => a.localeCompare(b)) + .join('\n\t- ')}` + : ''; + assert.strictEqual((e as Error).message, expectedMessage, `error thrown has correct message: ${expectedMessage}`); + } + + const expectedIds = expected.ids.join(','); + + assert.strictEqual( + record.friends.length, + expected.length, + `the new state has the correct length of ${expected.length} after ${mutation.method}` + ); + assert.deepEqual( + record.friends.slice(), + expected.membership, + `the new state has the correct records [${expectedIds}] after ${mutation.method} (had [${record.friends + .map((f) => f.id) + .join(',')}])` + ); + assert.deepEqual( + record.hasMany('friends').ids(), + expected.ids, + `the new state has the correct ids on the reference [${expectedIds}] after ${mutation.method}` + ); + assert.strictEqual( + record.hasMany('friends').ids().length, + expected.length, + `the new state has the correct length on the reference of ${expected.length} after ${mutation.method}` + ); + assert.strictEqual( + record.friends.length, + new Set(record.friends).size, + `the new state has no duplicates after ${mutation.method}` + ); + + await settled(); + + const start = mutation.start?.(record) ?? 0; + const deleteCount = mutation.deleteCount?.(record) ?? 0; + const isReplace = + mutation.method === 'splice' && (deleteCount > 0 || (start === 0 && deleteCount === record.friends.length)); + + if (shouldError || (!isReplace && initialIds === expectedIds)) { + assert.strictEqual(counters.friends, initialFriendsCount, 'reactivity: friendsCount does not increment'); + } else { + assert.strictEqual(counters.friends, initialFriendsCount + 1, 'reactivity: friendsCount increments'); + } + assert + .dom(`li:nth-child(${friendsIndex + 1})`) + .hasText(`friends: [${expectedIds}]`, 'reactivity: friends are rendered'); +} + +function getStartingState() { + return [ + { name: 'empty friends', cb: (store: Store) => makeUser(store, []) }, + { name: '1 friend', cb: (store: Store) => makeUser(store, [krystanRef()]) }, + { name: '2 friends', cb: (store: Store) => makeUser(store, [krystanRef(), samRef()]) }, + ]; +} + +function getValues() { + return [ + { + name: 'with empty array', + values: [], + }, + { + name: 'with NO duplicates (compared to initial remote state)', + values: [ericRef()], + }, + { + name: 'with duplicates NOT present in initial remote state', + values: [ericRef(), ericRef()], + }, + { + name: 'with duplicates present in initial remote state', + values: [krystanRef()], + }, + { + name: 'with all the duplicates', + values: [ericRef(), ericRef(), krystanRef()], + }, + ]; +} + +function generateMutations(baseMutation: Omit): Mutation[] { + return getValues().map((v) => ({ + ...baseMutation, + name: `${baseMutation.name} ${v.name}`, + values: v.values, + })); +} + +function getMutations(): Mutation[] { + return [ + ...generateMutations({ + name: 'push', + method: 'push', + }), + ...generateMutations({ + name: 'unshift', + method: 'unshift', + }), + ...generateMutations({ + name: 'replace', + method: 'splice', + start: () => 0, + deleteCount: (user) => user.friends.length, + }), + ...generateMutations({ + name: 'splice with delete (to beginning)', + method: 'splice', + start: () => 0, + deleteCount: (user) => (user.friends.length === 0 ? 0 : 1), + }), + ...generateMutations({ + name: 'splice (to beginning)', + method: 'splice', + start: () => 0, + deleteCount: () => 0, + }), + ...generateMutations({ + name: 'splice (to middle)', + method: 'splice', + start: (user) => Math.floor(user.friends.length / 2), + deleteCount: () => 0, + }), + ...generateMutations({ + name: 'splice (to end)', + method: 'splice', + start: (user) => user.friends.length, + deleteCount: () => 0, + }), + ]; +} + +module('Integration | Relationships | Collection | Mutation', function (hooks) { + setupRenderingTest(hooks); + + hooks.beforeEach(function () { + this.owner.register('model:user', User); + }); + + getStartingState().forEach((startingState) => { + module(`Starting state: ${startingState.name}`, function () { + getMutations().forEach((mutation) => { + module(`Mutation: ${mutation.name}`, function () { + getMutations().forEach((mutation2) => { + test(`followed by Mutation: ${mutation2.name}`, async function (assert) { + const store = this.owner.lookup('service:store') as Store; + const user = startingState.cb(store); + const rc = await reactiveContext.call(this, user, [{ name: 'friends', type: 'hasMany' }]); + rc.reset(); + + await applyMutation(assert, store, user, mutation, rc); + await applyMutation(assert, store, user, mutation2, rc); + }); + }); + }); + }); + }); + }); +}); diff --git a/tests/main/tests/integration/serializers/embedded-records-mixin-test.js b/tests/main/tests/integration/serializers/embedded-records-mixin-test.js index 6a2337d7aaa..35d2cb5dc76 100644 --- a/tests/main/tests/integration/serializers/embedded-records-mixin-test.js +++ b/tests/main/tests/integration/serializers/embedded-records-mixin-test.js @@ -1587,30 +1587,26 @@ module('integration/embedded-records-mixin', function (hooks) { const superVillain = store.createRecord('super-villain', { id: '1', firstName: 'Super', - lastName: 'Villian', + lastName: 'Villain', homePlanet, secretLab, }); - const secretWeapon = store.createRecord('secret-weapon', { + store.createRecord('secret-weapon', { id: '1', name: 'Secret Weapon', superVillain, }); - - superVillain.secretWeapons.push(secretWeapon); - - const evilMinion = store.createRecord('evil-minion', { + store.createRecord('evil-minion', { id: '1', name: 'Evil Minion', superVillain, }); - superVillain.evilMinions.push(evilMinion); const serializer = store.serializerFor('super-villain'); const serializedRestJson = serializer.serialize(superVillain._createSnapshot()); const expectedOutput = { firstName: 'Super', - lastName: 'Villian', + lastName: 'Villain', homePlanet: '123', evilMinions: [ { @@ -1677,29 +1673,26 @@ module('integration/embedded-records-mixin', function (hooks) { const superVillain = store.createRecord('super-villain', { id: '1', firstName: 'Super', - lastName: 'Villian', + lastName: 'Villain', homePlanet, secretLab, }); - const secretWeapon = store.createRecord('secret-weapon', { + store.createRecord('secret-weapon', { id: '1', name: 'Secret Weapon', superVillain, }); - - superVillain.secretWeapons.push(secretWeapon); - const evilMinion = store.createRecord('evil-minion', { + store.createRecord('evil-minion', { id: '1', name: 'Evil Minion', superVillain, }); - superVillain.evilMinions.push(evilMinion); const serializer = store.serializerFor('super-villain'); const serializedRestJson = serializer.serialize(superVillain._createSnapshot()); const expectedOutput = { firstName: 'Super', - lastName: 'Villian', + lastName: 'Villain', homePlanet: '123', evilMinions: [ { @@ -1923,27 +1916,24 @@ module('integration/embedded-records-mixin', function (hooks) { const superVillain = store.createRecord('super-villain', { id: '1', firstName: 'Super', - lastName: 'Villian', + lastName: 'Villain', }); - const evilMinion = store.createRecord('evil-minion', { + store.createRecord('evil-minion', { id: '1', name: 'Evil Minion', superVillain, }); - const secretWeapon = store.createRecord('secret-weapon', { + store.createRecord('secret-weapon', { id: '1', name: 'Secret Weapon', superVillain, }); - superVillain.evilMinions.push(evilMinion); - superVillain.secretWeapons.push(secretWeapon); - const serializer = store.serializerFor('super-villain'); const serializedRestJson = serializer.serialize(superVillain._createSnapshot()); const expectedOutput = { firstName: 'Super', - lastName: 'Villian', + lastName: 'Villain', homePlanet: null, evilMinions: [ { diff --git a/tests/main/tests/integration/snapshot-test.js b/tests/main/tests/integration/snapshot-test.js index f3f35d7cf0d..504ac3cfe2a 100644 --- a/tests/main/tests/integration/snapshot-test.js +++ b/tests/main/tests/integration/snapshot-test.js @@ -498,13 +498,32 @@ module('integration/snapshot - Snapshot', function (hooks) { }); test('snapshot.belongsTo() returns a snapshot if relationship link has been fetched', async function (assert) { - assert.expect(4); - store.adapterFor('application').findBelongsTo = function (store, snapshot, link, relationship) { - return Promise.resolve({ data: { id: '1', type: 'post', attributes: { title: 'Hello World' } } }); + return Promise.resolve({ + data: { + id: '1', + type: 'post', + attributes: { title: 'Hello World' }, + relationships: { comments: { links: { related: './comments' } } }, + }, + included: [ + { + type: 'comment', + id: '2', + relationships: { + post: { + data: { + type: 'post', + id: '1', + }, + }, + }, + }, + ], + }); }; - store.push({ + const comment = store.push({ data: { type: 'comment', id: '2', @@ -520,30 +539,14 @@ module('integration/snapshot - Snapshot', function (hooks) { }, }, }); - const comment = store.peekRecord('comment', '2'); - const post = await comment.post; - store.push({ - data: [ - { - type: 'post', - id: '1', - attributes: { - title: 'Hello World', - }, - }, - { - type: 'comment', - id: '2', - attributes: { - body: 'This is comment', - }, - }, - ], - }); + // test preconditions of + const initialCommentSnapshot = comment._createSnapshot(); + const initialBelongsTo = initialCommentSnapshot.belongsTo('post'); + assert.strictEqual(initialBelongsTo, undefined, 'relationship is empty'); - const comments = await post.comments; - comments.push(comment); + // fetch the link + const post = await comment.post; const postSnapshot = post._createSnapshot(); const commentSnapshot = comment._createSnapshot(); @@ -1294,7 +1297,7 @@ module('integration/snapshot - Snapshot', function (hooks) { type: 'posts', }, }; - assert.deepEqual(snapshot.serialize(), expected, 'shapshot serializes correctly'); + assert.deepEqual(snapshot.serialize(), expected, 'snapshot serializes correctly'); expected.data.id = '1'; assert.deepEqual(snapshot.serialize({ includeId: true }), expected, 'serialize takes options'); }); diff --git a/tests/main/tsconfig.json b/tests/main/tsconfig.json index b787097e7dd..37420592c8e 100644 --- a/tests/main/tsconfig.json +++ b/tests/main/tsconfig.json @@ -27,7 +27,9 @@ "declaration": false, "types": ["ember-source/types"], - "paths": {} + "paths": { + "@ember-data/deprecations": ["../../packages/private-build-infra/virtual-packages/deprecations.d.ts"] + } }, "references": [] } diff --git a/tests/warp-drive__schema-record/package.json b/tests/warp-drive__schema-record/package.json index 98bc3c87147..fed2cf4096c 100644 --- a/tests/warp-drive__schema-record/package.json +++ b/tests/warp-drive__schema-record/package.json @@ -64,11 +64,15 @@ }, "ember-inflector": { "injected": true + }, + "@ember-data/debug": { + "injected": true } }, "devDependencies": { "@babel/core": "^7.23.6", "@babel/runtime": "^7.23.6", + "@ember-data/debug": "workspace:5.5.0-alpha.11", "@ember-data/graph": "workspace:5.5.0-alpha.11", "@ember-data/json-api": "workspace:5.5.0-alpha.11", "@ember-data/private-build-infra": "workspace:5.5.0-alpha.11",