Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX release beta canary] Fix Model lifecycle event deprecations #6395

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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') {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this here was always an instance of InternalModel so this conditional was always false

deprecate(
`Attempted to define ${methodName} on ${this._record.modelName}#${this._record.id}`,
deprecations[methodName],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This never evaluated to true because we were never adding the found deprecated method to deprecations, so deprecations always had a value of {}

{
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there is a way to avoid creating these data structures in memory if a user doesn't define any of these at all. Perhaps a majority don't? What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely agree that the majority a model classes won't be defining any of these methods. I'm not sure if there is a way to avoid creating these but I'm not sure it matters much since this code will only run in development. It's not clear from this diff but this is all wrapped inside of an if (DEBUG)

}

return deprecations;
};

Model.reopen({
init() {
this._super(...arguments);

if (DEBUG) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

},
});
}
Expand Down