-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[BUGFIX release beta canary] Fix Model lifecycle event deprecations #6395
Conversation
be9b75b
to
80fe6a5
Compare
'ready', | ||
'rolledBack', | ||
].forEach(methodName => { | ||
if (this instanceof Model && typeof this._record[methodName] === 'function') { |
There was a problem hiding this comment.
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
Model.reopen({ | ||
init() { | ||
this._super(...arguments); | ||
|
||
if (DEBUG) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already wrapped an in if (DEBUG) {
on https://github.com/emberjs/data/pull/6395/files#diff-f80cb1aa960a031cbf31acf375a55b0aR1343
if (this instanceof Model && typeof this._record[methodName] === 'function') { | ||
deprecate( | ||
`Attempted to define ${methodName} on ${this._record.modelName}#${this._record.id}`, | ||
deprecations[methodName], |
There was a problem hiding this comment.
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 {}
a4d99d7
to
60de9b6
Compare
|
||
lifecycleDeprecations.add(methodName); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
if (!deprecations) { | ||
deprecations = new Set(); | ||
INSTANCE_DEPRECATIONS.set(instance, deprecations); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
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/); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like your tests 💯
Related RFC: https://github.com/emberjs/rfcs/blob/master/text/0329-deprecated-ember-evented-in-ember-data.md Original PR: emberjs#6059 Deprecations were added for the following model lifecycle events `becameError`, `becameInvalid`, `didCreate`, `didDelete`, `didLoad`, `didUpdate`, `ready`, `rolledBack` in emberjs#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 emberjs#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 emberjs#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.
60de9b6
to
d4c48d5
Compare
tl;dr;
Model
lifecycle event methodsModel
lifecycle event method deprecationsDeprecations 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 theModel
class, but that can never be true sincethis
will always be an instance ofInternalModel
:data/packages/store/addon/-private/system/model/internal-model.ts
Lines 446 to 455 in 5e49539
Since we only want to check for these deprecations for instances of
Model
, I moved the deprecation logic to theModel
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:
data/packages/store/addon/-private/system/model/internal-model.ts
Lines 61 to 77 in 5e49539
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, so the same deprecation would be logged multiple times.