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

Revert "unloadRecord does not remove observers" #4594

Merged

Conversation

pangratz
Copy link
Member

Reverts #4277

@ryanpatrickcook Looks like #4277 is not green on current master. Can you reopen a PR based on the latest master?

@pangratz pangratz merged commit a136f67 into master Oct 19, 2016
@pangratz pangratz deleted the revert-4277-Unload-Record-Does-Not-Remove-Observers branch October 19, 2016 16:03
@@ -122,8 +122,8 @@ var Model = Ember.Object.extend(Ember.Evented, {
@type {Boolean}
@readOnly
*/
hasDirtyAttributes: Ember.computed('currentState', function() {
return this.get('_internalModel.currentState.isDirty');
hasDirtyAttributes: Ember.computed('currentState.isDirty', function() {
Copy link
Member

Choose a reason for hiding this comment

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

this seems like it could be a Ember.computed.readOnly('currentState.isDirty')

@ryanpatrickcook
Copy link
Contributor

@pangratz bummer. Looks like it was failing on the alpha, beta, and canary builds. Seems like there was a change here that may have caused my issue.

After the destroy in unloadRecord, it seems to fail on initial access to get(record, 'isDeleted'), but subsequent attempts are ok.

screen shot 2016-10-19 at 3 02 26 pm

Will look into. Any help would be appreciated.

@pangratz
Copy link
Member Author

@ryanpatrickcook looks like there is similar work going on in #4593, which should address the whole unload situation at a broader level. So I am not sure if it's worth pursuing this further for now... Maybe we should wait until that PR is finished and re-check if the test in your PR is still failing or not.

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.

3 participants