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 release] Guard against isDestroyed in ManyArray.flushCanonical #3559

Merged
merged 1 commit into from
Mar 9, 2016
Merged

[BUGFIX release] Guard against isDestroyed in ManyArray.flushCanonical #3559

merged 1 commit into from
Mar 9, 2016

Conversation

jgwhite
Copy link
Contributor

@jgwhite jgwhite commented Jul 18, 2015

Addresses #3084

This is the simplest-possible-patch I could find to get around a real pain point for unloading records. It looks like you guys are doing more fundamental work to address this but I wonder if we could put a fix like this in place as a stopgap.

If it’s needless and the real fix is about to land, I totally understand. If you think it is worthwhile I’ll flesh it out and add tests.

@bmac
Copy link
Member

bmac commented Jul 20, 2015

Hey @jgwhite adding a test for this would be great!

@jgwhite
Copy link
Contributor Author

jgwhite commented Jul 20, 2015

@bmac test case added. The circumstances are kinda weird but it’s something I managed to run into in day-to-day development. I haven’t been able to go deeper on why a dead ManyArray is being referenced after unload but hopefully the test will help narrow it down.

@jgwhite
Copy link
Contributor Author

jgwhite commented Jul 20, 2015

Currently updating the test to new JSON API style.

@jgwhite
Copy link
Contributor Author

jgwhite commented Jul 20, 2015

Also just noticed I accidentally committed some changes further up in the file — my bad, was in a rush!

@jgwhite
Copy link
Contributor Author

jgwhite commented Jul 21, 2015

@bmac okay, now this is ready for review 😁

@bmac
Copy link
Member

bmac commented Jul 24, 2015

@igorT can you review this pr?

@sly7-7
Copy link
Contributor

sly7-7 commented Jul 24, 2015

@jgwhite could you try if this makes this test #3084 (comment) pass ?

@bmac
Copy link
Member

bmac commented Aug 3, 2015

ping @igorT

@jcope2013
Copy link
Contributor

what's the status on this?

jcope2013 added a commit to InboxHealth/data that referenced this pull request Aug 20, 2015
@jcope2013
Guard against isDestroyed in ManyArray.flushCanonical emberjs#3559
02efdd6
@jgwhite
Copy link
Contributor Author

jgwhite commented Sep 2, 2015

@sly7-7 just ran this patch against the test case in your comment and it does not fix it.

@jgwhite
Copy link
Contributor Author

jgwhite commented Sep 2, 2015

Just rebased.

@Leooo
Copy link

Leooo commented Sep 14, 2015

can this be merged?

@kevinyuliawan
Copy link

+1

jcope2013 added a commit to InboxHealth/data that referenced this pull request Sep 21, 2015
@jcope2013
Guard against isDestroyed in ManyArray.flushCanonical emberjs#3559
ba4c517
jcope2013 added a commit to InboxHealth/data that referenced this pull request Nov 18, 2015
@jcope2013
Guard against isDestroyed in ManyArray.flushCanonical emberjs#3559
e9216a7
@olivierchatry
Copy link

+1

@fivetanley
Copy link
Member

@jgwhite I'm inclined to merge this. There's a few things that need to happen before that can happen:

  1. rebase
  2. comment explaining why the guard check is necessary
  3. Test mentions issue unloadingRecord w/ relationships fail in flushCanonical #3084 in the test description

@jgwhite
Copy link
Contributor Author

jgwhite commented Mar 2, 2016

@fivetanley thanks for the guidance. I’ll make time to address those points today.

stefanpenner
stefanpenner reviewed Mar 2, 2016
View reviewed changes
packages/ember-data/lib/system/many-array.js
@@ -80,7 +80,9 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, {
toSet = toSet.concat(newRecords);
var oldLength = this.length;
this.arrayContentWillChange(0, this.length, toSet.length);
this.set('length', toSet.length);
if (!this.isDestroyed) {
Copy link
Member

Choose a reason for hiding this comment

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

also consider adding this.isDestroying

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe re-use https://github.com/emberjs/data/blob/v2.4.0/addon/-private/system/store/common.js#L23-L25 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pangratz even better, thanks!

@jgwhite
Copy link
Contributor Author

jgwhite commented Mar 4, 2016

@fivetanley not quite as quick as promised but points addressed and ready for another review :)

@jgwhite
Copy link
Contributor Author

jgwhite commented Mar 4, 2016

Looks like AppVeyor crashed with phantomjs not installed. Anything I can do to help out with that?

@stefanpenner
Copy link
Member

maybe something sideways here ?

will kick the appveyor build..

bmac added a commit that referenced this pull request Mar 9, 2016
@bmac
Merge pull request #3559 from jgwhite/flush-canonical-on-unload
11484c4 
[BUGFIX release] Guard against isDestroyed in ManyArray.flushCanonical
@bmac bmac merged commit 11484c4 into emberjs:master Mar 9, 2016
@bmac
Copy link
Member

bmac commented Mar 9, 2016

Thanks @jgwhite

@jgwhite
Copy link
Contributor Author

jgwhite commented Mar 9, 2016

😄

@jgwhite jgwhite deleted the flush-canonical-on-unload branch March 9, 2016 14:33
@jcope2013 jcope2013 mentioned this pull request Mar 16, 2016
Closed
jcope2013 added a commit to InboxHealth/data that referenced this pull request Aug 25, 2016
@jcope2013
Guard against isDestroyed in ManyArray.flushCanonical emberjs#3559
d772f90
@wecc wecc mentioned this pull request Oct 22, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@igorT igorT

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.