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

Data Adapter: Only trigger model type update if the record live array count actually changed #15604

Merged
merged 1 commit into from
Oct 4, 2017

Conversation

teddyzeenny
Copy link
Contributor

Fixes emberjs/ember-inspector#690

The issue:

In some cases, Ember Data's peekAll triggers arrayContentDidChange on the record array manager's live record array even if the records didn't change. One example case is when we call unloadRecord followed by peekAll within the same run loop.

The peekAll will trigger the array observer's didChange callback (with zero changes). This is generally harmless as long as we guard against that in our data adapter. Without the guard, we risk causing an infinite recursion because our didChange observer itself calls peekAll, which in the above scenario will re-trigger the observer and so on.

… count actually changed

Fixes emberjs/ember-inspector#690

The issue:

In some cases, Ember Data's `peekAll` triggers `arrayContentDidChange` on the record array manager's live record array even if the records didn't change.
One example case is when we call `unloadRecord` and then `peekAll` within the same run loop.
The `peekAll` will trigger the array observer's `didChange` callback (with zero changes).
This is generally harmless as long as we guard against that in our data adapter.
Without the guard, we risk causing an infinite recursion because our `didChange` observer itself calls `peekAll`, which in the above scenario will re-trigger
the observer and so on.
@nkgm
Copy link
Contributor

nkgm commented Sep 6, 2017

@rwjblue @stefanpenner could this make it into 2.14? Right now it has big negative impact on ember inspector.

@samselikoff
Copy link
Contributor

+1 on this, it's killing my dev workflow.

Is there a temporary workaround?

@xomaczar
Copy link
Contributor

xomaczar commented Oct 4, 2017

Chrome freezes for me too - running [email protected] and [email protected].

@rwjblue rwjblue merged commit 5c6333c into master Oct 4, 2017
@rwjblue rwjblue deleted the data-adapter-observer branch October 4, 2017 19:54
@rwjblue
Copy link
Member

rwjblue commented Oct 5, 2017

This was released in 2.15.2

@DLiblik
Copy link

DLiblik commented Oct 10, 2017

@rwjblue When will 2.15.2 release? This is causing an issue in one of our applications as well.

@locks
Copy link
Contributor

locks commented Oct 11, 2017

@DLiblik
Copy link

DLiblik commented Oct 11, 2017

@rwjblue My fault - poor question on my part: I meant through ember-cli. Running npm to install [email protected] fails on the version not being available. The releases there also do not list 2.15.2 https://github.com/ember-cli/ember-cli/releases - it might just be that I'm not clear how the two versioning processes sync up.

@Serabe
Copy link
Member

Serabe commented Oct 24, 2017

Ember can be upgraded without having to upgrade Ember CLI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants