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

live updated dirty tracking (attrs and rels) #140

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aars
Copy link
Collaborator

@aars aars commented Mar 6, 2017

_changedAttributes and _changedRelationships are Observable arrays, to allow live dirty tracking.

@aars
Copy link
Collaborator Author

aars commented Mar 6, 2017

I don't really like the way this is set up, creating the Ember.A([]) only when it is first used (in attr utility and _relationAdded and _relationRemoved), but the test suite is way to fragile for me to change all that (and, I might break some intentionally fragile test setups). For example, the attr test uses this as a Resource:
let Resource = Ember.Object.extend({ attributes: {}, _attributes: {} });

Of course I could add _changedAttributes etc to this "mock" resource, but I don't think thats a good way to build a test-suite. attr is not something that can or should be used without a Resource. It very much relies on the structure of Resource objects (hence the _attributes added to this Mock resource).

I'd like to discuss these changes/additions before continuing. Also, I'm going to use these changes for a bit, see if something else rears it head. For now this simply works for me.

@andrewmp1
Copy link
Collaborator

@aars This is great work. In working with this library originally I had been using proxy objects to track changes. Now I've been gradually moving towards handling those concerns in the resource as well and I've started to hit the same scenarios you are with this fix. When you have a moment can you fix the conflicts on this branch? I think its due to me merging in another PR where with the Ember.copy on attrs.

@aars
Copy link
Collaborator Author

aars commented Mar 7, 2017

Done.

I'll add some tests when I get the chance.

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.

2 participants