diff --git a/FEATURES.md b/FEATURES.md index 625ea6e2872..6b1ff4cb61d 100644 --- a/FEATURES.md +++ b/FEATURES.md @@ -10,3 +10,6 @@ for a detailed explanation. Flag description here. --> +* `EMBER_A_NON_MODIFYING` + + Corrects Ember.A to no longer modify the source array. This brings it more in line with documented behavior. diff --git a/packages/@ember/-internals/runtime/tests/system/native_array/a_test.js b/packages/@ember/-internals/runtime/tests/system/native_array/a_test.js index e9b72d65d1a..c5e368448df 100644 --- a/packages/@ember/-internals/runtime/tests/system/native_array/a_test.js +++ b/packages/@ember/-internals/runtime/tests/system/native_array/a_test.js @@ -1,5 +1,7 @@ -import EmberArray from '@ember/array'; +import EmberArray, { NativeArray } from '@ember/array'; import { A } from '@ember/array'; +import { ENV } from '@ember/-internals/environment'; +import { isEmberArray } from '@ember/array/-internals'; import { moduleFor, AbstractTestCase } from 'internal-test-helpers'; moduleFor( @@ -24,3 +26,43 @@ moduleFor( } } ); + +if (!ENV.EXTEND_PROTOTYPES.Array) { + moduleFor( + 'Ember.A without Extended Prototypes', + class extends AbstractTestCase { + ['@feature(EMBER_A_NON_MODIFYING) Ember.A does not modify original'](assert) { + let original = [1, 2]; + let proxy = A(original); + + assert.notOk(EmberArray.detect(original), 'EmberArray is not detected in the original'); + assert.ok(EmberArray.detect(proxy), 'EmberArray is detected in the proxy'); + + assert.notOk(NativeArray.detect(original), 'NativeArray is not detected in the original'); + assert.ok(NativeArray.detect(proxy), 'NativeArray is detected in the proxy'); + + assert.strictEqual(proxy.objectAt(1), 2, 'proxies to original array'); + + proxy.pushObject(3); + assert.deepEqual(original, [1, 2, 3], 'original array gets updated'); + + assert.notOk(isEmberArray(original), 'original is not EmberArray'); + assert.ok(isEmberArray(proxy), 'proxy is EmberArray'); + + assert.ok(Array.isArray(proxy), 'proxy is a native array'); + + proxy.pushObjects([4, 5]); + assert.deepEqual(original, [1, 2, 3, 4, 5], 'pushObjects works'); + } + + ['@feature(EMBER_A_NON_MODIFYING) Ember.A adds warnings about modification to original']() { + let original = [1, 2]; + A(original); + + expectAssertion(() => { + original.pushObject(1); + }); + } + } + ); +} diff --git a/packages/@ember/array/index.ts b/packages/@ember/array/index.ts index 4d49c9ae270..d213b47cb83 100644 --- a/packages/@ember/array/index.ts +++ b/packages/@ember/array/index.ts @@ -2,7 +2,7 @@ @module @ember/array */ import { DEBUG } from '@glimmer/env'; -import { PROXY_CONTENT } from '@ember/-internals/metal'; +import { ComputedProperty, descriptorForDecorator, PROXY_CONTENT } from '@ember/-internals/metal'; import { objectAt, replaceInNativeArray, @@ -22,6 +22,7 @@ import Observable from '@ember/object/observable'; import type { MethodNamesOf, MethodParams, MethodReturns } from '@ember/-internals/utility-types'; import type { ComputedPropertyCallback } from '@ember/-internals/metal'; import { isEmberArray, setEmberArray } from '@ember/array/-internals'; +import { EMBER_A_NON_MODIFYING } from '@ember/canary-features'; export { default as makeArray } from './lib/make-array'; @@ -1199,6 +1200,25 @@ interface EmberArray extends Enumerable { */ without(value: T): NativeArray; } + +const emberArrayBrackets = nonEnumerableComputed({ + get() { + return this; + }, + set(_key, value) { + this.replace(0, this.length, value); + return this; + }, +}); + +const emberArrayFirstObject = nonEnumerableComputed(function () { + return objectAt(this, 0); +}).readOnly(); + +const emberArrayLastObject = nonEnumerableComputed(function () { + return objectAt(this, this.length - 1); +}).readOnly(); + const EmberArray = Mixin.create(Enumerable, { init() { this._super(...arguments); @@ -1209,23 +1229,11 @@ const EmberArray = Mixin.create(Enumerable, { return indexes.map((idx) => objectAt(this, idx)); }, - '[]': nonEnumerableComputed({ - get() { - return this; - }, - set(_key, value) { - this.replace(0, this.length, value); - return this; - }, - }), + '[]': emberArrayBrackets, - firstObject: nonEnumerableComputed(function () { - return objectAt(this, 0); - }).readOnly(), + firstObject: emberArrayFirstObject, - lastObject: nonEnumerableComputed(function () { - return objectAt(this, this.length - 1); - }).readOnly(), + lastObject: emberArrayLastObject, // Add any extra methods to EmberArray that are native to the built-in Array. slice(beginIndex = 0, endIndex?: number) { @@ -2099,6 +2107,127 @@ NativeArray.keys().forEach((methodName) => { NativeArray = NativeArray.without(...ignore); +const NATIVE_ARRAY_METHODS = Object.freeze(NativeArray.keys()); + +function getCP(decorator: Function) { + let desc = descriptorForDecorator(decorator); + assert('[BUG] Expected descriptor with getter', desc instanceof ComputedProperty); + return desc; +} + +// NOTE: This code borrows heavily from https://github.com/tracked-tools/tracked-built-ins/blob/master/addon/src/-private/array.ts + +// Unfortunately, TypeScript's ability to do inference *or* type-checking in a +// `Proxy`'s body is very limited, so we have to use a number of casts `as any` +// to make the internal accesses work. The type safety of these is guaranteed at +// the *call site* instead of within the body: you cannot do `Array.blah` in TS, +// and it will blow up in JS in exactly the same way, so it is safe to assume +// that properties within the getter have the correct type in TS. +class EmberAProxy { + /** + * Creates an array from an iterable object. + * @param iterable An iterable object to convert to an array. + */ + static from(iterable: Iterable | ArrayLike): EmberAProxy { + return new EmberAProxy(Array.from(iterable)); + } + + static of(...arr: T[]): EmberAProxy { + return new EmberAProxy(arr); + } + + constructor(arr: T[] = []) { + let self = this; + + let proxy = new Proxy(arr, { + get(target, prop /*, _receiver */) { + // These nonEnumerableComputed properties need special handling + if (prop === '[]') { + return self.#bracketsCP.get(proxy, prop); + } else if (prop === 'firstObject') { + return self.#firstObjectCP.get(proxy, prop); + } else if (prop === 'lastObject') { + return self.#lastObjectCP.get(proxy, prop); + } else if ( + typeof prop === 'string' && + (NATIVE_ARRAY_METHODS.has(prop) || prop === '_super') + ) { + // If it's a NativeArray method, call it with the proxy as the target + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (self as any)[prop].bind(proxy); + } + + // Pass through every other property + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (target as any)[prop]; + }, + + set(target, prop, value /*, _receiver */) { + // These nonEnumerableComputed properties need special handling + if (prop === '[]') { + self.#bracketsCP.set(proxy, prop, value); + } else if (prop === 'firstObject') { + self.#firstObjectCP.set(proxy, prop, value); + } else if (prop === 'lastObject') { + self.#lastObjectCP.set(proxy, prop, value); + } else { + // Pass through every other property + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (target as any)[prop] = value; + } + + return true; + }, + + getPrototypeOf() { + return EmberAProxy.prototype; + }, + }) as EmberAProxy; + + // Call init so it's properly registered + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (self as any).init.call(proxy); + + return proxy; + } + + #bracketsCP = getCP(emberArrayBrackets); + #firstObjectCP = getCP(emberArrayFirstObject); + #lastObjectCP = getCP(emberArrayLastObject); +} + +// Apply NativeArray to the the EmberAProxy so that it appears as a NativeArray +// and so that the correct methods are available. +NativeArray.apply(EmberAProxy.prototype); + +// This rule is correctly in the general case, but it doesn't understand +// declaration merging, which is how we're using the interface here. This +// declaration says that `EmberAProxy` acts just like `Array`, but also has +// the properties declared via the `class` declaration above -- but without the +// cost of a subclass, which is much slower that the proxied array behavior. +// That is: a `EmberAProxy` *is* an `Array`, just with a proxy in front of +// accessors and setters, rather than a subclass of an `Array` which would be +// de-optimized by the browsers. +// +// eslint-disable-next-line @typescript-eslint/no-empty-interface +interface EmberAProxy extends NativeArray {} + +// Ensure instanceof works correctly +Object.setPrototypeOf(EmberAProxy.prototype, Array.prototype); + +const nativeArrayThrowers = Object.fromEntries( + Array.from(NativeArray.keys()).map((key) => { + return [ + key, + () => + assert( + `Ember.A was incorrectly modifying the original array. Do not call the EmberArray function ${key} on the original array. Instead, call it on the value returned by Ember.A.` + ), + ]; + }) +); +const ModifiedNativeArray = Mixin.create(nativeArrayThrowers); + let A: (arr?: Array) => NativeArray; if (ENV.EXTEND_PROTOTYPES.Array) { @@ -2124,8 +2253,18 @@ if (ENV.EXTEND_PROTOTYPES.Array) { // SAFETY: If it's a true native array and it is also an EmberArray then it should be an Ember NativeArray return arr as unknown as NativeArray; } else { - // SAFETY: This will return an NativeArray but TS can't infer that. - return NativeArray.apply(arr ?? []) as NativeArray; + if (EMBER_A_NON_MODIFYING) { + // Remove this in Ember 5.0. + if (DEBUG) { + if (arr) { + ModifiedNativeArray.apply(arr); + } + } + return new EmberAProxy(arr ?? []); + } else { + // SAFETY: This will return an NativeArray but TS can't infer that. + return NativeArray.apply(arr ?? []) as NativeArray; + } } }; } diff --git a/packages/@ember/canary-features/index.ts b/packages/@ember/canary-features/index.ts index 89132270986..1a1847433c2 100644 --- a/packages/@ember/canary-features/index.ts +++ b/packages/@ember/canary-features/index.ts @@ -12,7 +12,7 @@ import { ENV } from '@ember/-internals/environment'; */ export const DEFAULT_FEATURES = { - // FLAG_NAME: true/false + EMBER_A_NON_MODIFYING: null, }; /** @@ -53,14 +53,12 @@ export function isEnabled(feature: string): boolean { } } -// Uncomment the below when features are present: - -// function featureValue(value: null | boolean) { -// if (ENV.ENABLE_OPTIONAL_FEATURES && value === null) { -// return true; -// } +function featureValue(value: boolean | null) { + if (ENV.ENABLE_OPTIONAL_FEATURES && value === null) { + return true; + } -// return value; -// } + return value; +} -// export const FLAG_NAME = featureValue(FEATURES.FLAG_NAME); +export const EMBER_A_NON_MODIFYING = featureValue(FEATURES.EMBER_A_NON_MODIFYING);