From d3a7ecce7c2caf3aa135f891a1fcd120f6deed7f Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Tue, 28 Feb 2023 16:32:00 -0800 Subject: [PATCH] [BUGFIX LTS] Don't run getters while applying mixins This change ensures that getters are never evaluated while applying mixins. It relies on the fact that all getters (including undecorated ones) get converted into classic decorators when the mixin is originally created. --- .github/workflows/ci.yml | 2 +- .../runtime/tests/mixins/accessor_test.js | 56 +++++++++++++++++++ packages/@ember/array/index.ts | 10 ++-- packages/@ember/object/mixin.ts | 50 +++++++++-------- smoke-tests/ember-test-app/package.json | 2 +- 5 files changed, 91 insertions(+), 29 deletions(-) create mode 100644 packages/@ember/-internals/runtime/tests/mixins/accessor_test.js diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c20a33eb655..f33c53aae71 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -92,7 +92,7 @@ jobs: - uses: actions/checkout@v3 - uses: actions/setup-node@v2 with: - node-version: 12.x + node-version: 14.x cache: yarn - name: install dependencies run: yarn install --frozen-lockfile --non-interactive diff --git a/packages/@ember/-internals/runtime/tests/mixins/accessor_test.js b/packages/@ember/-internals/runtime/tests/mixins/accessor_test.js new file mode 100644 index 00000000000..e7d3e04cdd3 --- /dev/null +++ b/packages/@ember/-internals/runtime/tests/mixins/accessor_test.js @@ -0,0 +1,56 @@ +import EmberObject from '@ember/object'; +import { moduleFor, RenderingTestCase } from 'internal-test-helpers'; + +moduleFor( + 'runtime: Mixin Accessors', + class extends RenderingTestCase { + ['@test works with getters'](assert) { + let value = 'building'; + + let Base = EmberObject.extend({ + get foo() { + if (value === 'building') { + throw Error('base should not be called yet'); + } + + return "base's foo"; + }, + }); + + // force Base to be finalized so its properties will contain `foo` + Base.proto(); + + class Child extends Base { + get foo() { + if (value === 'building') { + throw Error('child should not be called yet'); + } + + return "child's foo"; + } + } + + Child.proto(); + + let Grandchild = Child.extend({ + get foo() { + if (value === 'building') { + throw Error('grandchild should not be called yet'); + } + + return value; + }, + }); + + let instance = Grandchild.create(); + + value = 'done building'; + + assert.equal(instance.foo, 'done building', 'getter defined correctly'); + + value = 'changed value'; + + assert.equal(instance.foo, 'changed value', 'the value is a real getter, not a snapshot'); + } + } +); diff --git a/packages/@ember/array/index.ts b/packages/@ember/array/index.ts index 3de3d1605e8..7eeb4e5dde7 100644 --- a/packages/@ember/array/index.ts +++ b/packages/@ember/array/index.ts @@ -1930,11 +1930,11 @@ let NativeArray = Mixin.create(MutableArray, Observable, { }); // Remove any methods implemented natively so we don't override them -const ignore = ['length']; -NativeArray.keys().forEach((methodName) => { +const ignore: string[] = ['length']; +NativeArray.keys().forEach((methodName: unknown): void => { // SAFETY: It's safe to read unknown properties from an object - if ((Array.prototype as any)[methodName]) { - ignore.push(methodName); + if (Reflect.has(Array.prototype, methodName as string)) { + ignore.push(methodName as string); } }); @@ -1963,7 +1963,7 @@ if (ENV.EXTEND_PROTOTYPES.Array) { if (isEmberArray(arr)) { // SAFETY: If it's a true native array and it is also an EmberArray then it should be an Ember NativeArray - return arr as 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; diff --git a/packages/@ember/object/mixin.ts b/packages/@ember/object/mixin.ts index d874914a250..c5f1af79790 100644 --- a/packages/@ember/object/mixin.ts +++ b/packages/@ember/object/mixin.ts @@ -1,5 +1,5 @@ /** -@module @ember/object/mixin +@module @ember/object */ import { INIT_FACTORY } from '@ember/-internals/container'; import type { Meta } from '@ember/-internals/meta'; @@ -10,32 +10,34 @@ import { DEBUG } from '@glimmer/env'; import { _WeakSet } from '@glimmer/util'; import type { ComputedDecorator, + ComputedDescriptor, ComputedPropertyGetter, ComputedPropertyObj, ComputedPropertySetter, - ComputedDescriptor, } from '@ember/-internals/metal'; import { ComputedProperty, + addObserver, + defineDecorator, + defineValue, descriptorForDecorator, + isClassicDecorator, makeComputedDecorator, nativeDescDecorator, - setUnprocessedMixins, - addObserver, removeObserver, revalidateObservers, - defineDecorator, - defineValue, + setUnprocessedMixins, } from '@ember/-internals/metal'; -import { addListener, removeListener } from '@ember/object/events'; +import { addListener, removeListener } from './events'; const a_concat = Array.prototype.concat; const { isArray } = Array; -function extractAccessors(properties: { [key: string]: any } | undefined) { +function extractAccessors(properties: { [key: string]: unknown } | undefined) { if (properties !== undefined) { for (let key of Object.keys(properties)) { - let desc = Object.getOwnPropertyDescriptor(properties, key)!; + let desc = Object.getOwnPropertyDescriptor(properties, key); + assert(`a property descriptor for ${key} must exist`, desc !== undefined); if (desc.get !== undefined || desc.set !== undefined) { Object.defineProperty(properties, key, { value: nativeDescDecorator(desc) }); @@ -225,7 +227,7 @@ function mergeMixins( keys: string[], keysWithSuper: string[] ): void { - let currentMixin; + let currentMixin: MixinLike | undefined; for (let i = 0; i < mixins.length; i++) { currentMixin = mixins[i]; @@ -299,12 +301,17 @@ function mergeProps( let desc = meta.peekDescriptors(key); if (desc === undefined) { - // The superclass did not have a CP, which means it may have - // observers or listeners on that property. - let prev = (values[key] = base[key]); - - if (typeof prev === 'function') { - updateObserversAndListeners(base, key, prev, false); + // If the value is a classic decorator, we don't want to actually + // access it, because that will execute the decorator while we're + // building the class. + if (!isClassicDecorator(value)) { + // The superclass did not have a CP, which means it may have + // observers or listeners on that property. + let prev = (values[key] = base[key]); + + if (typeof prev === 'function') { + updateObserversAndListeners(base, key, prev, false); + } } } else { descs[key] = desc; @@ -534,8 +541,6 @@ export default class Mixin { /** @internal */ _without: any[] | undefined; - declare [INIT_FACTORY]?: null; - /** @internal */ constructor(mixins: Mixin[] | undefined, properties?: { [key: string]: any }) { MIXINS.add(this); @@ -546,7 +551,7 @@ export default class Mixin { if (DEBUG) { // Eagerly add INIT_FACTORY to avoid issues in DEBUG as a result of Object.seal(mixin) - this[INIT_FACTORY] = null; + (this as Record)[INIT_FACTORY] = null; /* In debug builds, we seal mixins to help avoid performance pitfalls. @@ -738,15 +743,14 @@ function _detect(curMixin: Mixin, targetMixin: Mixin, seen = new Set()): boolean return false; } -function _keys(mixin: Mixin, ret = new Set(), seen = new Set()) { +function _keys(mixin: Mixin, ret = new Set(), seen = new Set()) { if (seen.has(mixin)) { return; } seen.add(mixin); if (mixin.properties) { - let props = Object.keys(mixin.properties); - for (let prop of props) { + for (const prop of Object.keys(mixin.properties)) { ret.add(prop); } } else if (mixin.mixins) { @@ -755,3 +759,5 @@ function _keys(mixin: Mixin, ret = new Set(), seen = new Set()) { return ret; } + +export { Mixin }; diff --git a/smoke-tests/ember-test-app/package.json b/smoke-tests/ember-test-app/package.json index 3fe46b8a49d..b98fe19c8d2 100644 --- a/smoke-tests/ember-test-app/package.json +++ b/smoke-tests/ember-test-app/package.json @@ -63,7 +63,7 @@ "webpack": "^5.65.0" }, "engines": { - "node": "12.* || 14.* || >= 16" + "node": "14.* || >= 16" }, "ember": { "edition": "octane"