From 3b4ab60f096ba65aa2174b58fad8d3c54e7aab49 Mon Sep 17 00:00:00 2001 From: Aaron Heesakkers Date: Wed, 1 Mar 2017 15:28:14 +0100 Subject: [PATCH 01/10] live updated dirty tracking (attrs and rels) --- addon/models/resource.js | 49 ++++++++++++++++++++++++++++++++++++++-- addon/utils/attr.js | 35 ++++++++++++++++++++++++++-- 2 files changed, 80 insertions(+), 4 deletions(-) diff --git a/addon/models/resource.js b/addon/models/resource.js index 2bd61a1..9aa9301 100644 --- a/addon/models/resource.js +++ b/addon/models/resource.js @@ -103,6 +103,21 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { */ _attributes: null, + /** + Array of changed attribute keys. + + The Actual changed values (previous, changed) are stored on _attributes, + but we can not use that object or it's keys to create computed properties + that respond to changes (i.e. no live isDirty property). + This array is a simple list of attributes that are not in their original + state, and can be used to track the (dirty-)state of the resource. + + @protected + @property _changedAttributes + @type Array + */ + _changedAttributes: null, + /** Hash of relationships that were changed @@ -112,6 +127,15 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { */ _relationships: null, + /** + Array of changed relationship keys. Same use as _changedAttributes. + + @protected + @property _changedRelationships + @type Array + */ + _changedRelationships: null, + /** Flag for new instance, e.g. not persisted @@ -139,7 +163,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { */ _updateRelationshipsData(relation, ids) { if (!Array.isArray(ids)) { - this._updateToOneRelationshipData(relation, ids); + this._updateToOneRelationshipData(relation, ids); } else { let existing = this._existingRelationshipData(relation); if (!existing.length) { @@ -178,7 +202,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { */ _replaceRelationshipsData(relation, ids) { if (!Array.isArray(ids)) { - this._updateToOneRelationshipData(relation, ids); + this._updateToOneRelationshipData(relation, ids); } else { let existing = this._existingRelationshipData(relation); if (!existing.length) { @@ -300,6 +324,16 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { ref.added.push({type: pluralize(relation), id: id}); } } + + if (!this.get('_changedRelationships')) { + this.set('_changedRelationships', Ember.A([])); + } + + if (!Ember.isEmpty(ref.added) || + !Ember.isEmpty(ref.removals) || + !Ember.isEmpty(ref.changed)) { + this.get('_changedRelationships').pushObject(relation); + } }, /** @@ -364,6 +398,15 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { ref.removals.pushObject({ type: pluralize(relation), id: id }); } } + + if (!this.get('_changedRelationships')) { + this.set('_changedRelationships', Ember.A([])); + } + if (!Ember.isEmpty(ref.added) || + !Ember.isEmpty(ref.removals) || + !Ember.isEmpty(ref.previous)) { + this.get('_changedRelationships').removeObject(relation); + } }, /** @@ -455,6 +498,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { delete this._attributes[attr]; } } + this.set('_changedAttributes', Ember.A([])); }, /** @@ -499,6 +543,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { delete this._relationships[attr]; } } + this.set('_changedRelationships', Ember.A([])); }, /** diff --git a/addon/utils/attr.js b/addon/utils/attr.js index 2c6bdbc..ed02727 100644 --- a/addon/utils/attr.js +++ b/addon/utils/attr.js @@ -63,18 +63,49 @@ export default function attr(type = 'any', mutable = true) { set: function (key, value) { const lastValue = this.get('attributes.' + key); + // Don't allow set on immutable values. if (!_mutable) { return immutableValue(key, value, lastValue); } + // Don't do anything if same value is set. if (value === lastValue) { return value; } + + // Check value type. assertType.call(this, key, value); + + // Set value. this.set('attributes.' + key, value); + + // Track changes. + // Only on non-isNew resources, which are 'dirty' be default if (!this.get('isNew')) { + // Initialize tracking object and array for this attribute. this._attributes[key] = this._attributes[key] || {}; + if (!this.get('_changedAttributes')) { + this.set('_changedAttributes', Ember.A([])); + } + + // Track change(d key) and store previous/changed value. + // We (Ember.)Copy values to `previous` and `changed` to prevent both + // being a reference to the same object (and thus never showing up on + // computed property 'changedAttributes') if (this._attributes[key].previous === undefined) { - this._attributes[key].previous = lastValue; + // Value changed for the first time. + this._attributes[key].previous = Ember.copy(lastValue, true); + this.get('_changedAttributes').pushObject(key); + } else { + // Value changed again. + if (this._attributes[key].previous === value) { + // Value reverted to previous. No longer dirty. Remove from tracking. + this.get('_changedAttributes').removeObject(key); + } else if (this.get('_changedAttributes').indexOf(key) === -1){ + // Value changed again, wasn't tracked anymore. Track it. + this.get('_changedAttributes').pushObject(key); + } } - this._attributes[key].changed = value; + + this._attributes[key].changed = Ember.copy(value, true); + let service = this.get('service'); if (service) { service.trigger('attributeChanged', this); From a041a5d3ffaff9ba821090e42fee8ee38972b451 Mon Sep 17 00:00:00 2001 From: Aaron Heesakkers Date: Tue, 7 Mar 2017 10:22:18 +0100 Subject: [PATCH 02/10] Only track relationship changes for non new resources --- addon/models/resource.js | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/addon/models/resource.js b/addon/models/resource.js index 9aa9301..c16e35a 100644 --- a/addon/models/resource.js +++ b/addon/models/resource.js @@ -325,14 +325,17 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { } } - if (!this.get('_changedRelationships')) { - this.set('_changedRelationships', Ember.A([])); - } + // Track relationship changes + if (!this.get('isNew')) { + if (!this.get('_changedRelationships')) { + this.set('_changedRelationships', Ember.A([])); + } - if (!Ember.isEmpty(ref.added) || - !Ember.isEmpty(ref.removals) || - !Ember.isEmpty(ref.changed)) { - this.get('_changedRelationships').pushObject(relation); + if (!Ember.isEmpty(ref.added) || + !Ember.isEmpty(ref.removals) || + !Ember.isEmpty(ref.changed)) { + this.get('_changedRelationships').pushObject(relation); + } } }, @@ -399,13 +402,16 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { } } - if (!this.get('_changedRelationships')) { - this.set('_changedRelationships', Ember.A([])); - } - if (!Ember.isEmpty(ref.added) || - !Ember.isEmpty(ref.removals) || - !Ember.isEmpty(ref.previous)) { - this.get('_changedRelationships').removeObject(relation); + // Track relationship changes. + if (!this.get('isNew')) { + if (!this.get('_changedRelationships')) { + this.set('_changedRelationships', Ember.A([])); + } + if (!Ember.isEmpty(ref.added) || + !Ember.isEmpty(ref.removals) || + !Ember.isEmpty(ref.previous)) { + this.get('_changedRelationships').removeObject(relation); + } } }, From 5f629a05777f885e28d2ff024fe6505e65cd34ef Mon Sep 17 00:00:00 2001 From: Aaron Heesakkers Date: Thu, 9 Mar 2017 11:41:27 +0100 Subject: [PATCH 03/10] didUpdateResource carefully --- addon/models/resource.js | 38 ++++++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/addon/models/resource.js b/addon/models/resource.js index 2bd61a1..678fb72 100644 --- a/addon/models/resource.js +++ b/addon/models/resource.js @@ -139,7 +139,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { */ _updateRelationshipsData(relation, ids) { if (!Array.isArray(ids)) { - this._updateToOneRelationshipData(relation, ids); + this._updateToOneRelationshipData(relation, ids); } else { let existing = this._existingRelationshipData(relation); if (!existing.length) { @@ -178,7 +178,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { */ _replaceRelationshipsData(relation, ids) { if (!Array.isArray(ids)) { - this._updateToOneRelationshipData(relation, ids); + this._updateToOneRelationshipData(relation, ids); } else { let existing = this._existingRelationshipData(relation); if (!existing.length) { @@ -525,16 +525,42 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { }, /** - Sets all payload properties on the resource and resets private _attributes - used for changed/previous tracking + Callback after Resource is updated client-side, or from server payload. @method didUpdateResource @param {Object} json the updated data for the resource */ didUpdateResource(json) { - if (this.get('id') !== json.id) { return; } - this.setProperties(json); + // Received payload does not have to represent the full resource as we know it + // client-side. Specifically, relationship data can be safely omitted in payload, + // but that does not invalidate the relationship data we have stored client-side. + // + // If we receive a payload we can expect all attributes are present. + // Only _update_ specific relationship, don't replace `relationships` in full. + if (json) { + // Replace attributes in full. + if (json.attributes) { + this.set('attributes', json.attributes); + } + if (json.relationships) { + for (let relation in json.relationships) { + let key = `relationships.${relation}`; + if (!this.get(key)) { + this.set(key, json.relationships[relation]); + } else { + let links = json.relationships[relation].links; + let data = json.relationships[relation].data; + + if (links) { this.set(`${key}.links`, links); } + if (data) { this.set(`${key}.data`, data); } + } + } + } + } + + // reset tracking. this._resetAttributes(); + this._resetRelationships(); }, /** From fedf7de42fab352f824f1c526af3d94e4c250700 Mon Sep 17 00:00:00 2001 From: Aaron Heesakkers Date: Thu, 9 Mar 2017 12:12:46 +0100 Subject: [PATCH 04/10] improve updateResource --- addon/adapters/application.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/addon/adapters/application.js b/addon/adapters/application.js index 7082963..984a8fe 100644 --- a/addon/adapters/application.js +++ b/addon/adapters/application.js @@ -232,22 +232,22 @@ export default Ember.Object.extend(Evented, { let url = resource.get('links.self') || this.get('url') + '/' + resource.get('id'); let json = this.serializer.serializeChanged(resource); let relationships = this.serializer.serializeRelationships(resource, includeRelationships); - if ((includeRelationships && - ((!json && !relationships) || (!json && relationships.length === 0))) || - (!includeRelationships && !json)) { + console.log(json, relationships); + if (Ember.isEmpty(json) && Ember.isEmpty(relationships)) { return RSVP.Promise.resolve(null); } + json = json || { data: { id: resource.get('id'), type: resource.get('type') } }; - let cleanup = Ember.K; + if (relationships) { json.data.relationships = relationships; - cleanup = resource._resetRelationships.bind(resource); } + return this.fetch(url, { method: 'PATCH', body: JSON.stringify(json), update: true - }).then(cleanup); + }).then(resource.didUpdateResource.bind(resource)); }, /** From 2d50840ea3dcb08f0d90374cf18ed1975e638dc5 Mon Sep 17 00:00:00 2001 From: Aaron Heesakkers Date: Thu, 9 Mar 2017 13:18:12 +0100 Subject: [PATCH 05/10] private (not tracked) and public (tracked) addRelationship methods. --- addon/models/resource.js | 83 +++++++++++++++++++++++++++++----------- 1 file changed, 60 insertions(+), 23 deletions(-) diff --git a/addon/models/resource.js b/addon/models/resource.js index 274c37e..a9824a1 100644 --- a/addon/models/resource.js +++ b/addon/models/resource.js @@ -167,9 +167,9 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { } else { let existing = this._existingRelationshipData(relation); if (!existing.length) { - this.addRelationships(relation, ids); + this._addRelationships(relation, ids); } else if (ids.length > existing.length) { - this.addRelationships(relation, unique(ids, existing)); + this._addRelationships(relation, unique(ids, existing)); } else if (existing.length > ids.length) { this.removeRelationships(relation, unique(existing, ids)); } @@ -189,7 +189,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { if (id === null || isType('string', id) && existing !== id) { this.removeRelationship(relation, existing); if (id !== null) { - this.addRelationship(relation, id); + this._addRelationship(relation, id); } } }, @@ -206,10 +206,10 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { } else { let existing = this._existingRelationshipData(relation); if (!existing.length) { - this.addRelationships(relation, ids); + this._addRelationships(relation, ids); } else { this.removeRelationships(relation, existing); - this.addRelationships(relation, ids); + this._addRelationships(relation, ids); } } }, @@ -235,6 +235,12 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { } }, + _addRelationships(related, ids) { + for (let i = 0; i < ids.length; i++) { + this._addRelationship(related, ids[i]); + } + }, + /** @method removeRelationships @param {String} related - resource name @@ -246,6 +252,35 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { } }, + /** + Public method for adding relationships, with dirty/change tracking. + + @method addRelationship + @param {String} related - resource name + @param {String} id + */ + addRelationship(related, id) { + if (id !== undefined) { id = id.toString(); } // ensure String id. + + let meta = this.relationMetadata(related); + let type = pluralize(meta.type); + let identifier = { type: type, id: id }; + let previous; + + // toOne could have a previous relation. + if (meta.kind === 'toOne') { + previous = this.get(`relationships.${meta.relation}.data.id`); + } + + this._relationAdded( + related, + identifier, + (previous) ? { type: type, id: previous } : null + ); + + this._addRelationship(related, id); + }, + /** Adds related resource identifier object to the relationship data. @@ -260,11 +295,12 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { - http://jsonapi.org/format/#document-resource-object-linkage - http://jsonapi.org/format/#document-resource-identifier-objects - @method addRelationship + @method _addRelationship @param {String} related - resource name @param {String} id */ - addRelationship(related, id) { + + _addRelationship(related, id) { if (id !== undefined) { id = id.toString(); } // ensure String id. // actual resource type of this relationship is found in related-proxy's meta. @@ -274,8 +310,8 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { let type = pluralize(meta.type); let identifier = { type: type, id: id }; let resource = getOwner(this).lookup(`service:${type}`).cacheLookup(id); + if (Array.isArray(data)) { - this._relationAdded(related, identifier); data.push(identifier); if (resource) { let resources = this.get(related); @@ -284,8 +320,6 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { } } } else { - let previous = (data && data.id) ? { type: type, id: data.id } : null; - this._relationAdded(related, identifier, previous); data = identifier; if (resource) { this.set(`${meta.relation}.content`, resource); @@ -309,14 +343,15 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { */ _relationAdded(relation, identifier, previous) { let meta = this.relationMetadata(relation); - setupRelationshipTracking.call(this, relation, meta.kind); - let ref = this._relationships[relation]; - let relationshipData = this.get(`relationships.${relation}.data`); + let ref = this._relationships[relation]; + + // TODO: Remove this? Why reset tracking? + // setupRelationshipTracking.call(this, relation, meta.kind); + // TODO: why look up data when we get passed `previous`? + // let relationshipData = this.get(`relationships.${relation}.data`); if (meta && meta.kind === 'toOne') { - if (!relationshipData || relationshipData.id !== identifier.id) { - ref.changed = identifier; - ref.previous = ref.previous || previous; - } + ref.changed = identifier; + ref.previous = ref.previous || previous; } else if (meta && meta.kind === 'toMany') { let id = identifier.id; ref.removals = Ember.A(ref.removals.rejectBy('id', id)); @@ -389,6 +424,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { @param {String} id */ _relationRemoved(relation, id) { + console.log('relation removed', this.get('type'), relation, id); let ref = this._relationships[relation] = this._relationships[relation] || {}; let meta = this.relationMetadata(relation); setupRelationshipTracking.call(this, relation, meta.kind); @@ -410,7 +446,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { if (!Ember.isEmpty(ref.added) || !Ember.isEmpty(ref.removals) || !Ember.isEmpty(ref.previous)) { - this.get('_changedRelationships').removeObject(relation); + this.get('_changedRelationships').pushObject(relation); } } }, @@ -457,10 +493,11 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { */ changedRelationships: computed('_relationships', { get() { - let relationships = Object.keys(this._relationships).filter( (relation) => { + let relationships = Object.keys(this._relationships).filter((relation) => { let ref = this._relationships[relation]; - return !!ref.changed || (ref.removals && ref.removals.length) || - (ref.added && ref.added.length); + // `changed` can be null when a relation is removed. Check for previous as well. + return (ref.previous || ref.changed) || + (!Ember.isEmpty(ref.removals) || !Ember.isEmpty(ref.added)); }); return Ember.A(relationships); } @@ -520,7 +557,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { let meta = this.relationMetadata(relation); if (meta && meta.kind === 'toOne') { if (ref.changed && ref.changed.id && ref.previous && ref.previous.id) { - this.addRelationship(relation, ref.previous.id); + this._addRelationship(relation, ref.previous.id); } } else if (meta && meta.kind === 'toMany') { let added = ref.added.mapBy('id'); @@ -529,7 +566,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { this.removeRelationship(relation, id); }); removed.forEach( (id) => { - this.addRelationship(relation, id); + this._addRelationship(relation, id); }); } }); From fb477346ec881a7e40b7d0159483fce3de347529 Mon Sep 17 00:00:00 2001 From: Aaron Heesakkers Date: Thu, 9 Mar 2017 15:33:30 +0100 Subject: [PATCH 06/10] move setupRelationshipTracking out of _relationAdded --- addon/models/resource.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/addon/models/resource.js b/addon/models/resource.js index a9824a1..b7499fc 100644 --- a/addon/models/resource.js +++ b/addon/models/resource.js @@ -345,9 +345,11 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { let meta = this.relationMetadata(relation); let ref = this._relationships[relation]; - // TODO: Remove this? Why reset tracking? + // FIXME: Why setup tracking here each time? I've moved the call + // to this._resetRelationships. // setupRelationshipTracking.call(this, relation, meta.kind); - // TODO: why look up data when we get passed `previous`? + + // FIXME: why look up data when we get passed `previous`? // let relationshipData = this.get(`relationships.${relation}.data`); if (meta && meta.kind === 'toOne') { ref.changed = identifier; @@ -581,11 +583,13 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { @method _resetRelationships */ _resetRelationships() { - for (let attr in this._relationships) { - if (this._relationships.hasOwnProperty(attr)) { - delete this._relationships[attr]; + for (let relation in this._relationships) { + if (this._relationships.hasOwnProperty(relation)) { + let meta = this.relationMetadata(relation); + setupRelationshipTracking.call(this, relation, meta.kind); } } + this.set('_changedRelationships', Ember.A([])); }, @@ -619,6 +623,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { @param {Object} json the updated data for the resource */ didUpdateResource(json) { + console.log('resource didUpdate', this.toString()); // Received payload does not have to represent the full resource as we know it // client-side. Specifically, relationship data can be safely omitted in payload, // but that does not invalidate the relationship data we have stored client-side. From 910dc2f79fded431ed925c2e21c4a232cdb5b4d2 Mon Sep 17 00:00:00 2001 From: Aaron Heesakkers Date: Thu, 9 Mar 2017 15:34:05 +0100 Subject: [PATCH 07/10] fetch() -> fetchResource(). Wrapper around ember-fetchjax change. --- addon/adapters/application.js | 42 +++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/addon/adapters/application.js b/addon/adapters/application.js index 984a8fe..544a8ab 100644 --- a/addon/adapters/application.js +++ b/addon/adapters/application.js @@ -77,7 +77,7 @@ export default Ember.Object.extend(Evented, { @param {Object} headers received from response @param {Object} options passed into original request */ - deserialize(json, headers, options={}) { + deserialize(json, headers, options = {}) { if (!json || json === '' || !json.data) { return null; } else if (options.isUpdate) { @@ -135,7 +135,7 @@ export default Ember.Object.extend(Evented, { findOne(id, query) { let url = this.get('url') + '/' + id; url += (query) ? '?' + Ember.$.param(query) : ''; - return this.fetch(url, { method: 'GET' }); + return this.fetchResource(url, { method: 'GET' }); }, /** @@ -149,7 +149,7 @@ export default Ember.Object.extend(Evented, { let url = this.get('url'); url += (options.query) ? '?' + Ember.$.param(options.query) : ''; options = options.options || { method: 'GET' }; - return this.fetch(url, options); + return this.fetchResource(url, options); }, /** @@ -182,7 +182,7 @@ export default Ember.Object.extend(Evented, { // use resource's service if in container, otherwise use this service to fetch let service = getOwner(this).lookup('service:' + pluralize(type)) || this; url = this.fetchUrl(url); - return service.fetch(url, { method: 'GET' }); + return service.fetchResource(url, { method: 'GET' }); }, /** @@ -194,7 +194,7 @@ export default Ember.Object.extend(Evented, { @return {Promise} */ createResource(resource) { - return this.fetch(this.get('url'), { + return this.fetchResource(this.get('url'), { method: 'POST', body: JSON.stringify(this.serializer.serialize(resource)) }).then(function(resp) { @@ -232,22 +232,30 @@ export default Ember.Object.extend(Evented, { let url = resource.get('links.self') || this.get('url') + '/' + resource.get('id'); let json = this.serializer.serializeChanged(resource); let relationships = this.serializer.serializeRelationships(resource, includeRelationships); - console.log(json, relationships); if (Ember.isEmpty(json) && Ember.isEmpty(relationships)) { return RSVP.Promise.resolve(null); } - json = json || { data: { id: resource.get('id'), type: resource.get('type') } }; - + if (!json) { + json = { data: { id: resource.get('id'), type: resource.get('type') } }; + } if (relationships) { json.data.relationships = relationships; } + // Successful responses get deserialized, except for 204 no content. return this.fetch(url, { method: 'PATCH', body: JSON.stringify(json), update: true - }).then(resource.didUpdateResource.bind(resource)); + }).then(({status}) => { + // A 204 (No Content) response will not trigger deserialization, and + // won't trigger cacheUpdate, which won't trigger didUpdateResource. + // Do that manually. + if (status === 204) { + resource.didUpdateResource(); + } + }); }, /** @@ -397,6 +405,22 @@ export default Ember.Object.extend(Evented, { return this._fetch(url, options); }, + /** + ember-fetchjax will resolve with {payload, status, headers, options}, + but most fetch calls here are only interested in the payload response (resource). + + @method fetchResource + @param {String} url + @param {Object} options + @return {Promise} + */ + fetchResource(url, options = {}) { + return new Ember.RSVP.Promise((resolve, reject) => { + this.fetch(url, options).then(({payload}) => { + resolve(payload); + }).catch(reject); + }); + }, /** Hook to customize the URL, e.g. if your API is behind a proxy and you need to swap a portion of the domain to make a request on the same domain. From e8d4f71567a4b09781330b5815730f40d6204962 Mon Sep 17 00:00:00 2001 From: Aaron Heesakkers Date: Fri, 10 Mar 2017 11:52:50 +0100 Subject: [PATCH 08/10] bugfix (remove called without existing) and improvement on removeRelationship. Debugging stuff --- addon/models/resource.js | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/addon/models/resource.js b/addon/models/resource.js index b7499fc..4dd0e72 100644 --- a/addon/models/resource.js +++ b/addon/models/resource.js @@ -8,7 +8,6 @@ import { pluralize, singularize } from 'ember-inflector'; import attr from 'ember-jsonapi-resources/utils/attr'; import { toOne, hasOne } from 'ember-jsonapi-resources/utils/to-one'; import { toMany, hasMany } from 'ember-jsonapi-resources/utils/to-many'; -import { isType } from 'ember-jsonapi-resources/utils/is'; import ResourceOperationsMixin from '../mixins/resource-operations'; const { getOwner, computed, Logger } = Ember; @@ -183,14 +182,22 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { @param {String|null} id */ _updateToOneRelationshipData(relation, id) { + if (id !== undefined) { id = id.toString(); } // ensure String id. + let relationshipData = 'relationships.' + relation + '.data'; let existing = this.get(relationshipData); existing = (existing) ? existing.id : null; - if (id === null || isType('string', id) && existing !== id) { + + // Nothing to remove or add, stop. + if ((!existing && id === null) || existing === id) { return; } + + // Remove old relationship. + if (existing) { this.removeRelationship(relation, existing); - if (id !== null) { - this._addRelationship(relation, id); - } + } + // Add new relationship? + if (id !== null) { + this._addRelationship(relation, id); } }, @@ -393,6 +400,10 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { @param {String} id */ removeRelationship(related, id) { + // Debug: I want to know when relationship removals are tracked. + Ember.Logger.debug('_relationRemoved', this.toString(), related, id); + console.trace(); + if (id !== undefined) { id = id.toString(); } // ensure String ids. let relation = this.get('relationships.' + related); if (Array.isArray(relation.data)) { @@ -426,10 +437,14 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { @param {String} id */ _relationRemoved(relation, id) { - console.log('relation removed', this.get('type'), relation, id); + // Debug: I want to know when relationship removals are tracked. + Ember.Logger.debug('_relationRemoved', this.toString(), relation, id); + let ref = this._relationships[relation] = this._relationships[relation] || {}; let meta = this.relationMetadata(relation); - setupRelationshipTracking.call(this, relation, meta.kind); + // FIXME: This is probably unnecesarry, tracking is setup on _reset. + // setupRelationshipTracking.call(this, relation, meta.kind); + if (meta.kind === 'toOne') { ref.changed = null; ref.previous = ref.previous || this.get('relationships.' + relation).data; @@ -623,18 +638,20 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { @param {Object} json the updated data for the resource */ didUpdateResource(json) { - console.log('resource didUpdate', this.toString()); + // Debug: I want to know when this callback is triggered. + Ember.Logger.debug('_relationRemoved', this.toString(), json); + // Received payload does not have to represent the full resource as we know it // client-side. Specifically, relationship data can be safely omitted in payload, // but that does not invalidate the relationship data we have stored client-side. // - // If we receive a payload we can expect all attributes are present. - // Only _update_ specific relationship, don't replace `relationships` in full. if (json) { + // If we receive a payload we can expect all attributes are present. // Replace attributes in full. if (json.attributes) { this.set('attributes', json.attributes); } + // Only _update_ specific relationship, don't replace `relationships` in full. if (json.relationships) { for (let relation in json.relationships) { let key = `relationships.${relation}`; From 719de37be1846293c98eec3ac09dac0b69d82f31 Mon Sep 17 00:00:00 2001 From: Aaron Heesakkers Date: Fri, 10 Mar 2017 13:05:49 +0100 Subject: [PATCH 09/10] correct debug messages --- addon/models/resource.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/addon/models/resource.js b/addon/models/resource.js index 4dd0e72..972caa6 100644 --- a/addon/models/resource.js +++ b/addon/models/resource.js @@ -401,7 +401,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { */ removeRelationship(related, id) { // Debug: I want to know when relationship removals are tracked. - Ember.Logger.debug('_relationRemoved', this.toString(), related, id); + Logger.debug('removeRelationship', this.toString(), related, id); console.trace(); if (id !== undefined) { id = id.toString(); } // ensure String ids. @@ -438,7 +438,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { */ _relationRemoved(relation, id) { // Debug: I want to know when relationship removals are tracked. - Ember.Logger.debug('_relationRemoved', this.toString(), relation, id); + Logger.debug('_relationRemoved', this.toString(), relation, id); let ref = this._relationships[relation] = this._relationships[relation] || {}; let meta = this.relationMetadata(relation); @@ -639,7 +639,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { */ didUpdateResource(json) { // Debug: I want to know when this callback is triggered. - Ember.Logger.debug('_relationRemoved', this.toString(), json); + Logger.debug('didUpdateResource', this.toString(), json); // Received payload does not have to represent the full resource as we know it // client-side. Specifically, relationship data can be safely omitted in payload, From 7276668371d7da409f81600af756a272ab8a4658 Mon Sep 17 00:00:00 2001 From: Aaron Heesakkers Date: Mon, 13 Mar 2017 10:39:21 +0100 Subject: [PATCH 10/10] setupRelationshipTracking resets. _relationAdded returns true if actually added (not new). Adds debug messages for tracking relationship issues --- addon/models/resource.js | 37 ++++++++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/addon/models/resource.js b/addon/models/resource.js index 972caa6..7f95c76 100644 --- a/addon/models/resource.js +++ b/addon/models/resource.js @@ -279,13 +279,19 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { previous = this.get(`relationships.${meta.relation}.data.id`); } - this._relationAdded( + console.log('addRelationship', this.toString(), related, id, previous); + + // _relationAdded will return true if this is an actual new/unknown + // relationship, and false if we already have this relationship. + let added = this._relationAdded( related, identifier, (previous) ? { type: type, id: previous } : null ); - this._addRelationship(related, id); + if (added) { + this._addRelationship(related, id); + } }, /** @@ -359,13 +365,29 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { // FIXME: why look up data when we get passed `previous`? // let relationshipData = this.get(`relationships.${relation}.data`); if (meta && meta.kind === 'toOne') { + // Relationship already exists? Nothing to do. + if (previous && previous.id === identifier.id) { + Logger.debug('_relationAdded', 'toOne', this.toString(), relation, identifier, 'already exists'); + return false; + } + + // Set changed/previous. ref.changed = identifier; ref.previous = ref.previous || previous; } else if (meta && meta.kind === 'toMany') { let id = identifier.id; + let type = pluralize(meta.type); + // Relationship already exists? Nothing to do. + if (this.relationships[type].data.findBy('id', id)) { + Logger.debug('_relationAdded', 'toMany', this.toString(), relation, identifier, 'already exists'); + return false; + } + + // If this id was part of removals, it should no longer be. ref.removals = Ember.A(ref.removals.rejectBy('id', id)); + // List as added relationship. if (!ref.added.findBy('id', id)) { - ref.added.push({type: pluralize(relation), id: id}); + ref.added.push({type: type, id: id}); } } @@ -381,6 +403,8 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { this.get('_changedRelationships').pushObject(relation); } } + + return true; }, /** @@ -640,6 +664,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, { didUpdateResource(json) { // Debug: I want to know when this callback is triggered. Logger.debug('didUpdateResource', this.toString(), json); + console.trace(); // Received payload does not have to represent the full resource as we know it // client-side. Specifically, relationship data can be safely omitted in payload, @@ -835,8 +860,9 @@ function setupRelationship(relation, kind) { } function setupRelationshipTracking(relation, kind) { - this._relationships[relation] = this._relationships[relation] || {}; - let ref = this._relationships[relation]; + //this._relationships[relation] = this._relationships[relation] || {}; + //let ref = this._relationships[relation]; + let ref = {}; if (kind === 'toOne') { ref.changed = ref.changed || null; ref.previous = ref.previous || null; @@ -844,6 +870,7 @@ function setupRelationshipTracking(relation, kind) { ref.added = ref.added || Ember.A([]); ref.removals = ref.removals || Ember.A([]); } + this._relationships[relation] = ref; } function unique(superSet, subSet) {