unloadRecord does not remove observers #4277

ryanpatrickcook · 2016-03-28T19:42:35Z

Issue: #4126

This is a work in progress to get conversation going on what the expectation should be. The test shows the issue where when you unload a record from the store, the observers on that object are not torn down.

Should unloadRecord set the object as destroyed?
Should unloadRecord clear observers?

bmac · 2016-03-28T22:15:51Z

@ryanpatrickcook I believe unloadRecord should destroy the object and clear observers.

ryanpatrickcook · 2016-03-28T23:10:14Z

That is what I would think as well, but it doesn't appear to be clearing observers. There is a twiddle in the issue linked that shows an example. In the test written here, it also appears that record.isDestroyed is false after unloading a record. Maybe the test is not accurate.

pangratz · 2016-04-09T15:36:00Z

Hey @ryanpatrickcook, this has been discussed in this weeks' team meeting and we decided that the record should indeed be destroyed when it's unloaded.

Sorry for the delay, but can you update this PR to do the following:

Destroy the record within store#unloadRecord()
Change this code so it re-uses the stores' unloadRecord so the record is destroyed too
Update the test so a) the setup (push to store, add observer), b) the destroying and c) the assertions are in a separate run-loop

After those changes the test should be green and this should be good to go! 🚀

pangratz · 2016-05-04T19:50:52Z

@ryanpatrickcook do you think you have time to update this PR? Happy to help or take over if you won't have time in the near future...

ryanpatrickcook · 2016-05-04T21:08:54Z

@pangratz I've updated with your input. Let me know what you think. Thanks!

ryanpatrickcook · 2016-05-04T22:12:44Z

tests/unit/store/unload-test.js

@@ -86,7 +86,7 @@ test("unload a record", function(assert) {
        store.unloadRecord(record);
      });

-      assert.equal(get(record, 'hasDirtyAttributes'), false, "record is not dirty");


Using Ember 1.13.13, after a record was unloaded/destroyed, get(record, 'hasDirtyAttributes') returned undefined. Since the record was now destroyed, I felt as though checking hasDirtyAttributes was no longer valid. Replaced with an isDestroyed check. Would it be preferred that hasDirtyAttributes responds with false?

Hmm, not sure what the expected behavior of hasDirtyAttributes is here. I will bring this up in the next meeting. Good point @ryanpatrickcook 👍

I just checked and it seems that hasDirtyAttributes is false after a record is destroyed (both for 1.13 and 2.5). See this ember-twiddle. Am I missing something?

I think in this case it would be ok to add back the assertion for hasDirtyAttributes...

In 1.13, it looks like if the assertion for hasDirtyAttributes is undefined when outside of the run-loop with unloadRecord. When inside the run-loop with unloadRecord, it returns false.

pangratz · 2016-05-15T18:59:08Z

tests/unit/store/unload-test.js

      });

-      assert.equal(get(record, 'hasDirtyAttributes'), false, "record is not dirty");


The reason this assertion is failing is due to the implementation of hasDirtyAttributes in DS.Model. It looks like a change to the currentState.isDirty dependency of the computed property is triggered when the record is unloaded.

@emberjs/ember-data-contributors are there any objections in changing the implementation so it re-uses the retrieveFromCurrentState function, like we already do for the other flags like isDirty?

@ryanpatrickcook so, we decided to change the implementation of the hasDirtyAttributes CP on the model, by aligning it to the implementation of the other flags. Can you change the dependent key here to be just currentState and add back the assertion. After that, this PR is good to go!

ryanpatrickcook · 2016-05-24T01:54:24Z

addon/-private/system/model/model.js

-  hasDirtyAttributes: Ember.computed('currentState.isDirty', function() {
-    return this.get('currentState.isDirty');
+  hasDirtyAttributes: Ember.computed('currentState', function() {
+    return this.get('_internalModel.currentState.isDirty');


@pangratz I updated the dependent key in the CP here to currentState, but the hasDirtyAttributes assertion still failed in 1.13 since this.get('currentState') is undefined. I updated the returned value to _internalModel.currentState.isDirty similar to what retrieveFromCurrentState is doing. Will this work?

How did you test this? Because we have an ember-try scenario for ember.js 1.13 and it looks like the test passes on Travis and AppVeyor.

@pangratz I switched the return back to this.get('currentState.isDirty'). Note the current failure on the branch. I think this should be returning this.get('_internalModel.currentState.isDirty'). In my previous question I was just wanting to verify with you that it was appropriate to refer to _internalModel.currentState.isDirty. That may have been confusing.

Ah, now I see. Sorry for being slow here. Re-reading your comment makes it clear what you meant. Yes, please change it back to this.get('_internalModel.currentState.isDirty'). Thanks!

Great! Thanks!

tchak · 2016-05-24T09:02:33Z

There is a PR #3301 to fix this in a more general way. Is it this old PR still relevant? cc @igorT

fivetanley · 2016-09-26T00:37:17Z

I think this is still relevant. @ryanpatrickcook can you rebase onto the latest master, and @pangratz can you take another look?

destroyed and observers removed

pangratz · 2016-10-19T14:56:04Z

Sorry this took so long @ryanpatrickcook. Thanks for your patience and this fix!

pangratz added the team-review label Mar 29, 2016

pangratz removed the team-review label Apr 9, 2016

ryanpatrickcook force-pushed the Unload-Record-Does-Not-Remove-Observers branch from c9bab2e to 9788587 Compare May 4, 2016 21:06

ryanpatrickcook force-pushed the Unload-Record-Does-Not-Remove-Observers branch from 9788587 to c7b827f Compare May 4, 2016 22:07

ryanpatrickcook reviewed May 4, 2016
View reviewed changes

pangratz added team-review and removed team-review labels May 8, 2016

ryanpatrickcook force-pushed the Unload-Record-Does-Not-Remove-Observers branch from c7b827f to 51fdcd6 Compare May 13, 2016 14:59

pangratz self-assigned this May 13, 2016

pangratz reviewed May 15, 2016
View reviewed changes

ryanpatrickcook force-pushed the Unload-Record-Does-Not-Remove-Observers branch from 51fdcd6 to 9ab9c07 Compare May 24, 2016 01:31

ryanpatrickcook changed the title ~~[wip] unloadRecord does not remove observers~~ unloadRecord does not remove observers May 24, 2016

ryanpatrickcook reviewed May 24, 2016
View reviewed changes

ryanpatrickcook force-pushed the Unload-Record-Does-Not-Remove-Observers branch 2 times, most recently from 056a72c to b38609f Compare May 24, 2016 13:48

store#unloadRecord to destroy record. add test to show record is

00e954c

destroyed and observers removed

ryanpatrickcook force-pushed the Unload-Record-Does-Not-Remove-Observers branch from b38609f to 00e954c Compare September 26, 2016 12:51

pangratz merged commit 26f9a57 into emberjs:master Oct 19, 2016

pangratz mentioned this pull request Oct 19, 2016

Revert "unloadRecord does not remove observers" #4594

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unloadRecord does not remove observers #4277

unloadRecord does not remove observers #4277

ryanpatrickcook commented Mar 28, 2016

bmac commented Mar 28, 2016

ryanpatrickcook commented Mar 28, 2016

pangratz commented Apr 9, 2016

pangratz commented May 4, 2016

ryanpatrickcook commented May 4, 2016

ryanpatrickcook May 4, 2016

pangratz May 8, 2016

pangratz May 9, 2016 •

edited

Loading

ryanpatrickcook May 13, 2016

pangratz May 15, 2016

pangratz May 23, 2016

ryanpatrickcook May 24, 2016

pangratz May 24, 2016

ryanpatrickcook May 24, 2016

pangratz May 24, 2016

ryanpatrickcook May 24, 2016

tchak commented May 24, 2016

fivetanley commented Sep 26, 2016

pangratz commented Oct 19, 2016

		});

		assert.equal(get(record, 'hasDirtyAttributes'), false, "record is not dirty");

unloadRecord does not remove observers #4277

unloadRecord does not remove observers #4277

Conversation

ryanpatrickcook commented Mar 28, 2016

bmac commented Mar 28, 2016

ryanpatrickcook commented Mar 28, 2016

pangratz commented Apr 9, 2016

pangratz commented May 4, 2016

ryanpatrickcook commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pangratz May 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchak commented May 24, 2016

fivetanley commented Sep 26, 2016

pangratz commented Oct 19, 2016

pangratz May 9, 2016 •

edited

Loading