arqex / freezer

A tree data structure that emits events on updates, even if the modification is triggered by one of the leaves, making it easier to think in a reactive way.
MIT License
1.28k stars 56 forks source link

Best way to remove an element from an array #33

Closed vegetabill closed 9 years ago

vegetabill commented 9 years ago

Hey there. I really like the API of your library.

But I was wondering your recommended way of removing items from arrays?

For nested objects, I've been doing something like (using underscore/lodash):

let remainingElements = _.without(parent.elements, elementToRemove);
parent.set('elements', remainingElements);

It seems to work but is that the best way to do it?

Then we ran into an issue when the array is the top-level object. We tried .reset but it didn't do what we expected.

let remainingElements = _.without(parent, elementToRemove);
parent.reset(remainingElements);
// ????

And we tried just creating a whole new top-level object, our app behaved really strangely since some objects had references to the old top-level object. We're probably doing something wrong.

let remainingElements = _.without(parent, elementToRemove);
parent = new Freezer(remainingElements);
// bad things happen
// maybe it's bad to create a freezer from frozen objects?
arqex commented 9 years ago

Hi @rearadmiral

It is nice that you use freezer with lodash, I always thought that this compatibilty is a strong point of freezer.

You need to always use the frozen nodes that are in the freezer store if you don't want to have sync problems. The best way of doing it is not keeping local copies of the nodes, try to use the store dirctly instead.

Besides that suggestion, I have created a jsbin to let try your code and with an alternative way of removing elements from arrays:

// Arrays are not supposed to be at
// the top level of a freezer store,
// But let's try!
var dataStore = new Freezer([
  1, 2, 3, 4, 5
]);

var data = dataStore.get();

data = data.reset(_.without( data, 3 ));

console.log( data ); // [1,2,4,5]

// It seems to work nicely for me,
// An alternative way
data = data.splice( data.indexOf(4), 1 );

console.log( data ); // [1,2,5]

http://jsbin.com/romaxubeke/2/edit?html,js,console

vegetabill commented 9 years ago

Thanks for replying.

But this just removes it from the returned copy. How do I remove it from the dataStore?

I had a feeling we weren't meant to use an array as the top-level element but if it's not recommended I'd suggest documenting that along with why because it seemed to work otherwise.

arqex commented 9 years ago

Oh you are right! Nice catch. Even if Freezer wasn't thought to store an array as root, I think it would be best to handle this case. I will fix it ASAP.

Thanks

arqex commented 9 years ago

Hi @rearadmiral This has been fixed in v0.5.2 thanks for your help!