From d4c48d52dac9af60d7e1f89af7e29885656a0d9e Mon Sep 17 00:00:00 2001 From: Eric Kelly Date: Thu, 29 Aug 2019 18:20:52 -0700 Subject: [PATCH] [BUGFIX release beta canary] Fix Model lifecycle event deprecations Related RFC: https://github.com/emberjs/rfcs/blob/master/text/0329-deprecated-ember-evented-in-ember-data.md Original PR: https://github.com/emberjs/data/pull/6059 Deprecations were added for the following model lifecycle events `becameError`, `becameInvalid`, `didCreate`, `didDelete`, `didLoad`, `didUpdate`, `ready`, `rolledBack` in https://github.com/emberjs/data/pull/6059. There was a discussion on the PR that suggested adding an additional check to make sure that that the deprecations were only triggered for instances of Ember Data's `Model` class https://github.com/emberjs/data/pull/6059#discussion_r284921979 This extra check was added but it accidentally was checking for `this` to be an instance of the `Model` class, but that can never be true since `this` will always be an instance of `InternalModel`: https://github.com/emberjs/data/blob/5e495394cbf67025be2622f604e67004d79bc21c/packages/store/addon/-private/system/model/internal-model.ts#L446-L455 Since we only want to check for these deprecations for instances of `Model`, I moved the deprecation logic to the `Model` class https://github.com/emberjs/data/pull/6059 also included a mechanism for tracking deprecations that were logged per model class so we could prevent logging multiple deprecations for the same model class and deprecated lifecycle method pair: https://github.com/emberjs/data/blob/5e495394cbf67025be2622f604e67004d79bc21c/packages/store/addon/-private/system/model/internal-model.ts#L61-L77 This also fixes a bug with the deprecation tracking logic which was that we were never actually adding the logged deprecations to the `WeakMap` meant to keep track of them. --- .eslintrc.js | 1 + packages/-ember-data/tests/unit/model-test.js | 28 ++++++++++++ .../-private/system/model/internal-model.ts | 43 ------------------ .../addon/-private/system/model/model.js | 45 +++++++++++++++++-- 4 files changed, 71 insertions(+), 46 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index afbec593094..7093cc78166 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -36,6 +36,7 @@ module.exports = { heimdall: true, Map: false, WeakMap: true, + Set: true, }, env: { browser: true, diff --git a/packages/-ember-data/tests/unit/model-test.js b/packages/-ember-data/tests/unit/model-test.js index ad8ddd397e5..f01c4b3077a 100644 --- a/packages/-ember-data/tests/unit/model-test.js +++ b/packages/-ember-data/tests/unit/model-test.js @@ -665,6 +665,34 @@ module('unit/model - Model', function(hooks) { assert.equal(eventMethodArgs[1], 2); } ); + + testInDebug('defining record lifecycle event methods on a model class is deprecated', async function(assert) { + class EngineerModel extends Model { + becameError() {} + becameInvalid() {} + didCreate() {} + didDelete() {} + didLoad() {} + didUpdate() {} + ready() {} + rolledBack() {} + } + + this.owner.register('model:engineer', EngineerModel); + + let store = this.owner.lookup('service:store'); + + store.createRecord('engineer'); + + assert.expectDeprecation(/You defined a `becameError` method for model:engineer but lifecycle events/); + assert.expectDeprecation(/You defined a `becameInvalid` method for model:engineer but lifecycle events/); + assert.expectDeprecation(/You defined a `didCreate` method for model:engineer but lifecycle events/); + assert.expectDeprecation(/You defined a `didDelete` method for model:engineer but lifecycle events/); + assert.expectDeprecation(/You defined a `didLoad` method for model:engineer but lifecycle events/); + assert.expectDeprecation(/You defined a `didUpdate` method for model:engineer but lifecycle events/); + assert.expectDeprecation(/You defined a `ready` method for model:engineer but lifecycle events/); + assert.expectDeprecation(/You defined a `rolledBack` method for model:engineer but lifecycle events/); + }); }); module('Reserved Props', function() { diff --git a/packages/store/addon/-private/system/model/internal-model.ts b/packages/store/addon/-private/system/model/internal-model.ts index 2c464149907..b57b335760a 100644 --- a/packages/store/addon/-private/system/model/internal-model.ts +++ b/packages/store/addon/-private/system/model/internal-model.ts @@ -58,24 +58,6 @@ interface BelongsToMetaWrapper { modelName: string; } -let INSTANCE_DEPRECATIONS; -let lookupDeprecations; - -if (DEBUG) { - INSTANCE_DEPRECATIONS = new WeakMap(); - - lookupDeprecations = function lookupInstanceDrecations(instance) { - let deprecations = INSTANCE_DEPRECATIONS.get(instance); - - if (!deprecations) { - deprecations = {}; - INSTANCE_DEPRECATIONS.set(instance, deprecations); - } - - return deprecations; - }; -} - /* The TransitionChainMap caches the `state.enters`, `state.setups`, and final state reached when transitioning from one state to another, so that future transitions can replay the @@ -430,31 +412,6 @@ export default class InternalModel { this._record = store._modelFactoryFor(this.modelName).create(createOptions); setRecordIdentifier(this._record, this.identifier); - if (DEBUG) { - let klass = this._record.constructor; - let deprecations = lookupDeprecations(klass); - [ - 'becameError', - 'becameInvalid', - 'didCreate', - 'didDelete', - 'didLoad', - 'didUpdate', - 'ready', - 'rolledBack', - ].forEach(methodName => { - if (this instanceof Model && typeof this._record[methodName] === 'function') { - deprecate( - `Attempted to define ${methodName} on ${this._record.modelName}#${this._record.id}`, - deprecations[methodName], - { - id: 'ember-data:record-lifecycle-event-methods', - until: '4.0', - } - ); - } - }); - } } } this._triggerDeferredTriggers(); diff --git a/packages/store/addon/-private/system/model/model.js b/packages/store/addon/-private/system/model/model.js index 3d95ab4129c..4a45beb2b22 100644 --- a/packages/store/addon/-private/system/model/model.js +++ b/packages/store/addon/-private/system/model/model.js @@ -1363,13 +1363,34 @@ if (DEBUG) { return isBasicDesc(instanceDesc) && lookupDescriptor(obj.constructor, keyName) === null; }; + const INSTANCE_DEPRECATIONS = new WeakMap(); + const DEPRECATED_LIFECYCLE_EVENT_METHODS = [ + 'becameError', + 'becameInvalid', + 'didCreate', + 'didDelete', + 'didLoad', + 'didUpdate', + 'ready', + 'rolledBack', + ]; + + let lookupDeprecations = function lookupInstanceDeprecations(instance) { + let deprecations = INSTANCE_DEPRECATIONS.get(instance); + + if (!deprecations) { + deprecations = new Set(); + INSTANCE_DEPRECATIONS.set(instance, deprecations); + } + + return deprecations; + }; + Model.reopen({ init() { this._super(...arguments); - if (DEBUG) { - this._getDeprecatedEventedInfo = () => `${this._internalModel.modelName}#${this.id}`; - } + this._getDeprecatedEventedInfo = () => `${this._internalModel.modelName}#${this.id}`; if (!isDefaultEmptyDescriptor(this, '_internalModel') || !(this._internalModel instanceof InternalModel)) { throw new Error( @@ -1393,6 +1414,24 @@ if (DEBUG) { `You may not set 'id' as an attribute on your model. Please remove any lines that look like: \`id: DS.attr('')\` from ${this.constructor.toString()}` ); } + + let lifecycleDeprecations = lookupDeprecations(this.constructor); + + DEPRECATED_LIFECYCLE_EVENT_METHODS.forEach(methodName => { + if (typeof this[methodName] === 'function' && !lifecycleDeprecations.has(methodName)) { + deprecate( + `You defined a \`${methodName}\` method for ${this.constructor.toString()} but lifecycle events for models have been deprecated.`, + false, + { + id: 'ember-data:record-lifecycle-event-methods', + until: '4.0', + url: 'https://deprecations.emberjs.com/ember-data/v3.x#toc_record-lifecycle-event-methods', + } + ); + + lifecycleDeprecations.add(methodName); + } + }); }, }); }