-
-
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
Make _recordData lazy #5767
Make _recordData lazy #5767
Conversation
addon/-private/system/snapshot.js
Outdated
@@ -65,7 +65,12 @@ export default class Snapshot { | |||
*/ | |||
this.modelName = internalModel.modelName; | |||
|
|||
this._changedAttributes = internalModel.changedAttributes(); | |||
if (internalModel.isLoading()) { |
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.
It seems like this logic belongs in internalModel
.
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.
@igorT seems fine; you'd prefer to push this into internalModel.changedAttributes()
?
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'm slightly in favor of snapshot since it is request specific logic, but I'm not so heavily in favor as to oppose it being in InternalModel.
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 don't think it is request-specific logic; if you have an internal model for a findRecord
(ie where isLoading() && !isReloading
), what does it even mean conceptually to ask for changed attributes?
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 I reorder that you may see what I mean a little bit better: "logic specific to requests"
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.
Looks good after comments addressed
3f958e7
to
4b070d6
Compare
Empty state should work
…On Thu, Nov 29, 2018, 18:06 David J. Hamilton ***@***.*** wrote:
***@***.**** commented on this pull request.
------------------------------
In addon/-private/system/snapshot.js
<#5767 (comment)>:
> @@ -65,7 +65,12 @@ export default class Snapshot {
*/
this.modelName = internalModel.modelName;
- this._changedAttributes = internalModel.changedAttributes();
+ if (internalModel.isLoading()) {
@runspired <https://github.com/runspired> what's your preferred branch?
the state here is root.loading
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5767 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AArpp8PPu3DRgHOcB1QATxRyEJIRpSeTks5u0JKSgaJpZM4Y63DP>
.
|
@igorT no, |
we could change |
@igorT never mind, @runspired pointed me to the |
4b070d6
to
d6616eb
Compare
Relevant for @igorT emberjs/rfcs#359 |
[BUGFIX beta] backport #5767 Make _recordData lazy
In particular, do not instantiate them for
findRecord