Remove items

[sirkitree]

Thanks for these examples, they're super helpful to study.

Most todo apps have a method to remove items, and I'm having a hard time figuring out how to do this with reactfire. Is this something that could be added to the examples? Or could you point me in the right direction and I'll create a pull request?

Thanks for any time you can spare on this.

[jwngr]

Hey there, thanks for the compliment and thanks for using the mixin!

The ReactFire mixin handles one way flow from Firebase to your component. In your case, you want to update Firebase directly by using the remove() method. If you want to create a PR with a delete button, I would add a method to TodoApp3 that looks something like this:

handleDelete: function(itemId) {
  var itemsRef = new Firebase("https://ReactFireTodoApp.firebaseio.com/items/");
  itemsRef.child(itemId).remove(function(error) {
    if (error) {
      console.log(error);
    }
  });
}

I think the hard part here will being finding the itemId for the item to delete. The mixin currently doesn't tell you the Firebase ID used to store an array item. This means that even if you have the item, you can't really delete it from Firebase since you don't know where it is stored. I think this is a shortcoming on the mixin. We do a better job of handling this with our other bindings (AngularFire, EmberFire, etc.). I'll see if I can whip together a better solution here which would make deleting easier (since it totally should be). If you want to take a crack at it, I'd be happy to discuss any of your code.

[sirkitree]

Thanks @jwngr!

I ended up accomplishing this in my own app by utilizing bindAsObject instead of bindAsArray which retrieves the keys, and then pass the key around as an attribute where needed. This makes it slightly less convenient to work with the data since it's an object and not an array (for instance, you can't just get the items.length) but at least I had the key.

If that is an acceptable approach (perhaps there is a better one I don't know about yet) then I'll work up a pull request.

[mhuebert]

@jwngr I'm running into a similar issue in an app I'm working on. In the case of an array of objects, I wouldn't mind storing the key of each element as its ".key" property. So a converted array could look like:

[ { title: "Hello",    ".key": ...key...}, 
  { title: "Good-Bye", ".key": ...key...} ]

Example code: https://github.com/mhuebert/reactfire/compare/firebase:master...patch-2

However, if elements of the array are not objects... I'm not sure what to do in the case of data that looks like ["a", "b", "c"]. We could expand them into objects.. [{".key": 0, ".value": "a"}, {".key": 1, ".value": "b"}, {".key": 2, ".value": "c"}]. We could return two arrays, of values and keys, and look up keys by index... ["a", "b", "c"], [0, 1, 2]. Neither seems clean. Maybe the solutions is to not use bindAsArray. Thoughts?

Also, looking at the code... since the "value" callback is used, and then we loop through the keys of the object to add them to the array, is there any guarantee of the order of the elements in the array? (vs. child_added?)

[sirkitree]

Yeah I was thinking about doing that after I got the objects too, converting to an array in a different manner that retained the key as a value. It would be nice if reactfire just handled that for us though.

[bs1180]

Hi, I ran into this issue as well, and submitted a simple PR similar to that proposed by @mhuebert, although I called the property _ref instead of .key.

[jwngr]

@sirkitree - I'm glad you were able to work around the issue! Personally, I think the example should remain as an array, not an object. We really just need to make ReactFire powerful enough to actually handle that use case. @bs1180 is off to a good start with his PR. I made a few comments on his approach, but hopefully between all of us, we can get this together.

@mhuebert - Thanks for the suggestion! Your approach is almost identical to what we do with AngularFire. Although not the cleanest, it has worked out pretty well and I think is what we should do with ReactFire.

As for your comment about the array order, that is a great point. The current implementation is not correct. We should either (1) make use of the DataSnapshot.forEach() method to properly iterate over the snapshot in order or (2) make use of the child_added, child_removed, and child_changed events instead of the value event. I think (1) makes sense in the short term and (2) makes the most sense in the long term. We should be able to get some performance benefits by doing (2) since then we won't be recreating so many records over and over every time an item is added to the array. I opened #36 to track this. Thanks for reporting!

[jwngr]

I also opened #37 to track the work for adding the Firebase keys to array items.

[jwngr]

Hey everybody - all of these issues are now resolved with today's 0.5.0 release! I even updated the live Todo app demo to include deleting items. You can find the source code in the examples/ directory. Let me know what you think!