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

ManyArray.flushCanonical should use set method for currentState #2763

Closed
wants to merge 1 commit into from

Conversation

MarSik
Copy link

@MarSik MarSik commented Feb 7, 2015

I was getting an "should use set" exception from ManyArray's
flushCannonical. This patch fixes the issue and works properly
in my application.

I was getting an "should use set" exception from ManyArray's
flushCannonical. This patch fixes the issue and works properly
in my application.
@bmac
Copy link
Member

bmac commented Feb 8, 2015

@MarSik the fact that you are getting a "should use set" error likely means you have a .get('currentState') somewhere in your codebase. Since currentState is a private property and isn't meant to be observable I believe the current code is correct.

@igorT knows this code best so he should make the final call on this pr.

@MarSik
Copy link
Author

MarSik commented Feb 8, 2015

I do not call get like that anywhere, but git grep on ember-data tells me:

packages/ember-data/lib/system/model/model.js:19: return get(get(this, 'currentState'), key);
packages/ember-data/lib/system/model/model.js:518: var currentState = get(this, 'currentState');
packages/ember-data/lib/system/model/model.js:537: var currentState = get(this, 'currentState');
packages/ember-data/lib/system/model/states.js:39: record.get('currentState.stateName');
packages/ember-data/lib/system/store.js:1187: if (get(record, 'currentState.stateName') === 'root.deleted.saved') {

So I supposed that meant flushCanonical should have used set.

    1. 2015 v 17:47, Brendan McLoughlin notifications@github.com:

@MarSik the fact that you are getting a "should use set" error likely means you have a .get('currentState') somewhere in your codebase. Since currentState is a private property and isn't meant to be observable I believe the current code is correct.

@igorT knows this code best so he should make the final call on this pr.


Reply to this email directly or view it on GitHub.

@bmac
Copy link
Member

bmac commented Feb 8, 2015

It looks like all of those lines turned up by grep are acting on the currentState property of a Model instance and not the currentState property of a hasMany relationship.

@MarSik
Copy link
Author

MarSik commented Feb 8, 2015

Yes, you are right. I was not accessing currentState directly anywhere though. Except from the Ember Firefox plugin when I was trying to debug my application. That might have triggered it as it probably uses getters for displaying fields.

    1. 2015 v 20:19, Brendan McLoughlin notifications@github.com:

It looks like all of those lines turned up by grep are acting on the currentState property of a Model instance and not the currentState property of a hasMany relationship.


Reply to this email directly or view it on GitHub.

@bmac
Copy link
Member

bmac commented Mar 1, 2015

I'm going to close this because it sounds like the original issue was caused by using the firefox plugin using a getter. Feel free to comment if there is still an issue and we can reopen.

@bmac bmac closed this Mar 1, 2015
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.

None yet

2 participants