Skip to content

Setting same property value still sets isDirty (Regression) - #646

Closed
workmanw wants to merge 1 commit into
warp-drive-data:masterfrom
Batterii:setting-same-value-marks-dirty
Closed

workmanw wants to merge 1 commit into
warp-drive-data:masterfrom
Batterii:setting-same-value-marks-dirty

Conversation

@workmanw

Copy link
Copy Markdown

This was marked as fixed in #221. In fact, I confirmed this to be fixed for @NickFranceschina, but it appears to have since regressed back to broken. Here is a pull request with a test against this issue.

My initial thought was to check in dirtyRecordsForAttributeChange against the data.attribute[attributeName] value, though I'm not confident about this. If someone can suggest a proper fix, I'd be happy to pursue it.

@workmanw

Copy link
Copy Markdown
Author

Oh yea, here is a fiddle: http://jsfiddle.net/y49ch/34/

@lcoq

lcoq commented Jan 23, 2013

Copy link
Copy Markdown

I have encountered this problem too.

I think the problem comes from the oldValue parameter of the DS.Adapter#dirtyRecordsForAttributeChange function, which is the last value set, instead of the last server value (available through record.get('data.attributes.' + attributeName)).

Consequently, the condition if (newValue !== oldValue) is always false, so the record stay dirty.

@lcoq

lcoq commented Jan 23, 2013

Copy link
Copy Markdown

After digging deeper, I begin to see the workflow. I post it here, maybe it could help...

When an attribute value changes, store#recordAttributeDidChange is called, and the adapter#dirtyRecordsForRecordChange add the record to the dirtySet.
Then, the store notify the record it has to be dirty. I don't see anywhere something that makes the record clean when the new value is set to the previous.

I tried to replace the willSetProperty from this:

context.oldValue = get(get(manager, 'record'), context.name);

to this:

context.oldValue = get(get(manager, 'record'), 'data.attributes.' + context.name);

But when the attribute value changes to its initial value, nothing happened because this condition has no else.

The dirtySet does not seems to be stored anywhere, so I have no idea how to know if the object is dirty only because of this attribute (and should consequently be cleaned), or if there are other changes (and should consequently stay dirty).

Hope it helps...

@bobbus

bobbus commented Jan 24, 2013

Copy link
Copy Markdown
Contributor

Actually, there is no way to find what caused a record to become dirty. The dirtySet is stored in a transaction but the transaction does not know what did change on the record, when it is rollback, it send the record to the state "loaded.materializing" to materialize again the last data received from the server.
Not sure how this bug should be handle, would love to hear from the core team about it.

cc @wycats

@workmanw

Copy link
Copy Markdown
Author

@bobbus, that's the conclusion I've come to also. I've spent the last few days learning the ins and outs of ember-data. As far as I can tell, there are really only three ways to make this work.

  1. Every time a property is changed and the newValue equals the original value (data.attributes.val) you'd need to iterate all of the attributes and determine if they're clean or dirty by comparing them to their original value.
  2. Similar to A, but instead of comparing the original value, you could have the DS.attr keep track of whether it's dirty or not.
  3. Each record, or maybe the record's adapter would keep a map of attributes that are dirty. Making it easy to lookup if anything is dirty.

I don't love any of those options, but that's what I came up with. I would definitely love to hear from one of the core guys on this.

@fsmanuel

fsmanuel commented Mar 6, 2013

Copy link
Copy Markdown
Contributor

@workmanw
i like your last approach. it opens a way to send only dirty data to the API. that is what i'm about to implement in our adapter...

@igorT

igorT commented Apr 8, 2013

Copy link
Copy Markdown
Contributor

This is getting fixed by #840

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.

5 participants