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

Improve property change notifications for records #2998

Closed
wants to merge 1 commit into from

Conversation

wecc
Copy link
Contributor

@wecc wecc commented Apr 13, 2015

This PR improves how we trigger property changes when pushing data to the store/a record.

Previously we only cared about comparing new data with model._data, this resulted in weird behavior in a couple of scenarios:

  1. If you modify an attribute locally and push new canonical data that matched the current local attribute we got a false trigger.
  2. If you modify an attribute locally and save, and the server return new canonical data that matches the existing canonical data, we did not get a trigger (hence the old locally changed value stayed).

Fixes #2937, fixes #2900

@wecc wecc force-pushed the record-property-changes branch from 1591366 to 244db37 Compare April 13, 2015 19:06
var length = keys.length;

original = merge({}, this._data);
original = merge(original, this._attributes);
Copy link
Member

Choose a reason for hiding this comment

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

not sure this is correct, as if you consider user applying changes after sending to the server, even if server sends a different value, it will not show up once you get it as an attr, as the local change will win

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we're in the middle of a save we have values in �_inFlightAttributes and that will win over _attributes�.

var person = store.push('person', { id: 1, name: 'Tomster' });
person.set('name', 'Yehuda');
person.save();
// => request { name: 'Yehuda' }
person.set('name', 'Tom');
// <= response { name: 'Yehuda' }

_data will contain { name: 'Tomster' }
_attributes will contain { name: 'Tom' }
_inFlightAttributes will contain { name: 'Yehuda' }

We will compare payload ({ name: 'Yehuda' }) to the merged original ({ name: 'Yehuda' }) and trigger no change.

Isn't this the correct behavior? We have { name: 'Tom' } locally (since we changed that) and we still do, without triggering observer. If we want to get back to { name: 'Yehuda' } we should do a rollback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, if the server were to return something completely different, say { name: 'Igor' }, we would get a trigger, but the value would remain { name: 'Tom' }. That might be an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but we would get even if you didn't merge in attributes, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Hm, then the order seems wrong, it should be inflight then attributes?

var keys = Ember.keys(updates);
var length = keys.length;

original = merge({}, this._data);
Copy link
Member

Choose a reason for hiding this comment

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

Ember.create(null) instead of {}

@igorT igorT mentioned this pull request Jun 2, 2015
28 tasks
@bmac
Copy link
Member

bmac commented Jun 5, 2015

@wecc what needs to happen for this to get merged?

@wecc
Copy link
Contributor Author

wecc commented Jun 5, 2015

@bmac needs to address the comments in here to make sure it's working as intended.

@igorT
Copy link
Member

igorT commented Jun 7, 2015

@bmac didn't you move this into a new pr?

@wecc
Copy link
Contributor Author

wecc commented Jun 8, 2015

@igorT it's in #3216

@igorT igorT closed this Jun 8, 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
4 participants