Removing objects from datastore #28

skinnerdev · 2016-03-10T20:54:28Z

I saw issue #26 and realized we had this logic on a fork of this project. Here is the code we've implemented to provide this functionality.

I've added a property to data store models that holds information about other models that reference it. This property is used during delete to assist with the complete removal of this object from the store. Any time a model with references is added to the store, we touch the references and "inform" them that they have been referenced.

The delete process uses the data in the associations array to find other objects in the store that reference the model to be deleted. If the deleted model is referenced with a hasMany relationship, the model to be deleted is removed from the referencing model's relationship array. If the reference is a hasOne/belongsTo relationship, the referencing model's relationship is given a new blank datastore model with only the _type property set.

…er models that reference it. This property is used during delete to assist with the complete removal of this object from the store.

beauby · 2016-03-10T21:01:03Z

Thanks a lot for this, looks solid to me. Will review it ASAP.

Cheers,

On Thursday, 10 March 2016, David Skinner notifications@github.com wrote:

I saw issue #26 #26
and realized we had this logic on a fork of this project. Here is the code
we've implemented to provide this functionality.

I've added a property to data store models that holds information about
other models that reference it. This property is used during delete to
assist with the complete removal of this object from the store. Any time a
model with references is added to the store, we touch the references and
"inform" them that they have been referenced.

The delete process uses the data in the associations array to find other
objects in the store that reference the model to be deleted. If the deleted
model is referenced with a hasMany relationship, the model to be deleted is
removed from the referencing model's relationship array. If the reference
is a hasOne/belongsTo relationship, the referencing model's relationship is

given a new blank datastore model with only the _type property set.

You can view, comment on, or merge this pull request online at:

#28
Commit Summary

Added a property to datastore models that holds information about
other models that reference it. This property is used during delete to
assist with the complete removal of this object from the store.

File Changes

M src/jsonapi-datastore.js
https://github.com/beauby/jsonapi-datastore/pull/28/files#diff-0
(23)

Patch Links:

https://github.com/beauby/jsonapi-datastore/pull/28.patch

https://github.com/beauby/jsonapi-datastore/pull/28.diff

—
Reply to this email directly or view it on GitHub
#28.

Lucas Hosseini
lucas.hosseini@gmail.com

beauby · 2016-03-10T23:50:03Z

src/jsonapi-datastore.js

+          }
+        });
+      } else if (self.graph[assoc.type][assoc.id][assoc.relation].id === model.id) {
+        self.graph[assoc.type][assoc.id][assoc.relation] = new JsonApiDataStoreModel(self.graph[assoc.type][assoc.id][assoc.relation]._type);


Why not make it null here?

It certainly could be null. I think I originally had it as null, but due to how we are using the data store, we found it useful to have the blank model there. I guess it could go either way. For a more global approach do you think null would be best?

I think it would be more consistent with what we do there.

beauby · 2016-03-11T00:13:59Z

Awesome, this feature is clearly a must! A few minor remarks, but I'd love to merge that ASAP.

skinnerdev · 2016-03-11T00:55:57Z

I've pushed up the changes as per our conversation thus far. On the property name issue, I think _dependents is fairly descriptive. Though if someone has a better idea for a name I'd be happy to change it.

beauby · 2016-03-11T00:58:18Z

src/jsonapi-datastore.js

-            model[key] = findOrInit(rel.data);
+            var ref = findOrInit(rel.data);
+            ref._dependents.push({id: model.id, type: model._type, relation: key});
+            model[key] = ref;


Nitpicking: how about

model[key] = findOrInit(rel.data); model[key]._dependents.push(...);

instead?

beauby · 2016-03-11T01:01:43Z

Great, thanks! _dependents seems good to me. Last thing needed would be one or two tests. I could probably write some tomorrow if you don't have time.

…array.

skinnerdev · 2016-03-11T01:04:38Z

The original work I had done on this is on another computer with my testing environment set up. I don't have a testing setup on this machine. If I have some time next time I am by it, I'll try and write some. Though might be a few days... :\

beauby · 2016-03-11T01:08:33Z

No worries, I can do it. Note that running the tests should just be a matter of

$ npm install
$ gulp test

…ing a relationship to only add to the _dependents array IF the model hasn't been referenced before. Delete will also remove the pointer in the _dependents array when the model is deleted.

skinnerdev · 2016-03-11T02:29:00Z

Thanks for the reminder! I had some extra time and setup the tests on this machine. I went and added a test and discovered a few tweaks that would help keep it clean. Let me know if there are any adjustments needed. :)

beauby · 2016-03-11T11:44:41Z

DOCUMENTATION.md

@@ -1,6 +1,6 @@


-<!-- Start src/jsonapi-datastore.js -->
+<!-- Start src\jsonapi-datastore.js -->


Any reason for this change?

Guess my machine automatically changed that when I ran gulp. I made sure it got reverted.

beauby · 2016-03-11T12:54:36Z

Nice, I think we're really close now, a tiny bit of cleanup and we should be good to merge!

…ependence.

skinnerdev · 2016-03-11T16:16:20Z

Added the _addDependence and _removeDependence.

beauby · 2016-03-11T21:01:48Z

Awesome, will review ASAP!

beauby · 2016-03-14T14:14:43Z

src/jsonapi-datastore.js

+      found;
+
+    found = self._dependents.filter(function(dependent) {
+      return dependent.id === id && dependent.type === type;


Here we should check for relationship key equality as well, as a model may have several back-references from a same model on different relationships (think post has_one author, and post has_many commenters).

…hod.

beauby · 2016-03-15T03:20:32Z

src/jsonapi-datastore.js

+    */
+  removeRelationship(type, id, relName) {
+    var self = this;
+    self._removeDependence(type, id);


By doing this we are effectively removing all dependences to the model, although we should probably just remove the dependence for the current relationship (which implies that _removeDependence would have to take a key parameter as well).

beauby · 2016-03-15T03:25:08Z

src/jsonapi-datastore.js

+    var self = this,
+      found;
+
+    self._dependents.forEach(function(val, idx) {


Here we could use find since we want to guarantee that those records are unique (provided we add a key parameter to the _removeDependence method). It would be slightly longer, but I believe would make the purpose of the code clearer.

beauby · 2016-10-21T01:35:47Z

Hey @skinnerdev – any news on this front?

Added a property to datastore models that holds information about oth…

c5510a3

…er models that reference it. This property is used during delete to assist with the complete removal of this object from the store.

beauby reviewed Mar 10, 2016
View reviewed changes

Renamed associations to dependents. Optimized usage of map vs forEach.

36cfd07

beauby reviewed Mar 11, 2016
View reviewed changes

Small adjustment to dependent assignment when relationship is not an …

d7e33a7

…array.

Added test for full delete functionality. Also added checks when sync…

f3af432

…ing a relationship to only add to the _dependents array IF the model hasn't been referenced before. Delete will also remove the pointer in the _dependents array when the model is deleted.

beauby reviewed Mar 11, 2016
View reviewed changes

Cleaned up the code and added new methods _addDependence and _removeD…

efdb707

…ependence.

beauby reviewed Mar 14, 2016
View reviewed changes

David Skinner added 2 commits March 14, 2016 09:35

Moved logic that removes relationship during delete to a separate met…

bab2dd7

…hod.

Changed filter to find when adding a dependent model.

c0d3309

beauby reviewed Mar 15, 2016
View reviewed changes

beauby mentioned this pull request Mar 15, 2016

Roadmap to 1.0 #29

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing objects from datastore #28

Removing objects from datastore #28

skinnerdev commented Mar 10, 2016

beauby commented Mar 10, 2016

given a new blank datastore model with only the _type property set.

beauby Mar 10, 2016

skinnerdev Mar 11, 2016

beauby Mar 11, 2016

beauby commented Mar 11, 2016

skinnerdev commented Mar 11, 2016

beauby Mar 11, 2016

skinnerdev Mar 11, 2016

beauby commented Mar 11, 2016

skinnerdev commented Mar 11, 2016

beauby commented Mar 11, 2016

skinnerdev commented Mar 11, 2016

beauby Mar 11, 2016

skinnerdev Mar 11, 2016

beauby commented Mar 11, 2016

skinnerdev commented Mar 11, 2016

beauby commented Mar 11, 2016

beauby Mar 14, 2016

beauby Mar 15, 2016

beauby Mar 15, 2016

beauby commented Oct 21, 2016

		@@ -1,6 +1,6 @@


		<!-- Start src/jsonapi-datastore.js -->
		<!-- Start src\jsonapi-datastore.js -->

Removing objects from datastore #28

Are you sure you want to change the base?

Removing objects from datastore #28

Conversation

skinnerdev commented Mar 10, 2016

beauby commented Mar 10, 2016

given a new blank datastore model with only the _type property set.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beauby commented Mar 11, 2016

skinnerdev commented Mar 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beauby commented Mar 11, 2016

skinnerdev commented Mar 11, 2016

beauby commented Mar 11, 2016

skinnerdev commented Mar 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beauby commented Mar 11, 2016

skinnerdev commented Mar 11, 2016

beauby commented Mar 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beauby commented Oct 21, 2016