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.

Sign up for GitHub

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

Jump to bottom

[BUGFIX #4649 #4648] #4643

Merged
merged 1 commit into from
Nov 14, 2016
Merged

[BUGFIX #4649 #4648] #4643

merged 1 commit into from
Nov 14, 2016

Conversation

stefanpenner
Copy link
Member

@stefanpenner stefanpenner commented Nov 11, 2016 •
edited
Loading

No longer warn/error in glimmer 2 by accidentally causing duplicate work.

stefanpenner
stefanpenner commented Nov 11, 2016
View reviewed changes
addon/-private/system/relationships/has-many.js
this.notifyPropertyChange(key);
//
let cache = Ember.meta(this).readableCache();
if (typeof cache === 'object' && cache !== null && cache[key] !== undefined) {
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 is just a quick experiment, don't mind the lolcode it is not currently my intention to merge this as is.

@stefanpenner stefanpenner mentioned this pull request Nov 11, 2016
Merged
@stefanpenner
[BUGFIX #4649 #4648]
b3e1d67 
* no longer invalid relationships property when accessing it the first time #4649
* no longer duplicate  “notifyHasManyChange” when adding a record fixes #4648

No longer warn/error in glimmer 2 by accidentally causing duplicate work.
@stefanpenner stefanpenner changed the title [BUGFIX] Quick Hack: lazyRelationships should not emit change events … [BUGFIX #4649 #4648] Nov 11, 2016
@stefanpenner stefanpenner added the concept-review Tracks PRs that introduce new concepts that need reviewed for discussion label Nov 12, 2016
@bmac
Copy link
Member

bmac commented Nov 13, 2016

👍 looks good to me.

@igorT igorT merged commit 3093191 into master Nov 14, 2016
@igorT igorT deleted the fix-stuff branch November 14, 2016 01:00
@runspired
Copy link
Contributor

For future note, dropping this line from ManyArray.internalReplace changed an undocumented and unintentional behavior that was public facing: b3e1d67#diff-f2ea099eb6bd7f8ec1eeff2bd7a32c6bL178

this.relationship.notifyHasManyChanged();

Before this, computed properties that subscribed to the relationship key (e.g. record.foos not records.foos.length or record.foos.[]) would dirty when internalReplace was called. While this is a bug and a performance issue we should fix, I believe this behavior change should go into the changelog. cc @bmac

@bmac bmac mentioned this pull request Nov 16, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
concept-review Tracks PRs that introduce new concepts that need reviewed for discussion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HasMany.addRecord notifies more then once
4 participants
@stefanpenner @bmac @runspired @igorT