Skip to content

Commit

Permalink
[BUGFIX release beta canary] Fix Model lifecycle event deprecations
Browse files Browse the repository at this point in the history
Related RFC: https://github.com/emberjs/rfcs/blob/master/text/0329-deprecated-ember-evented-in-ember-data.md
Original PR: #6059

Deprecations were added for the following model lifecycle events
`becameError`, `becameInvalid`, `didCreate`, `didDelete`, `didLoad`,
`didUpdate`, `ready`, `rolledBack` in #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 #6059 (comment)

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

#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.
  • Loading branch information
HeroicEric committed Sep 2, 2019
1 parent 5e49539 commit d4c48d5
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 46 deletions.
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ module.exports = {
heimdall: true,
Map: false,
WeakMap: true,
Set: true,
},
env: {
browser: true,
Expand Down
28 changes: 28 additions & 0 deletions packages/-ember-data/tests/unit/model-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
43 changes: 0 additions & 43 deletions packages/store/addon/-private/system/model/internal-model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand Down
45 changes: 42 additions & 3 deletions packages/store/addon/-private/system/model/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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('<type>')\` 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);
}
});
},
});
}
Expand Down

0 comments on commit d4c48d5

Please sign in to comment.