tsironis / lockr

A minimal API wrapper for localStorage
MIT License
815 stars 79 forks source link

how to remove an object from collection #26

Open hungcaoduy opened 8 years ago

hungcaoduy commented 8 years ago

I added my product into shopping cart by using:

Lockr.sadd("cart", {id: 123, name: abc}); Lockr.sadd("cart", {id: 456, name: def});

How to remove a particular object from cart? e.g I later want to remove the first object {id: 123, name: abc}

thanks

choval commented 8 years ago
var carts = Lockr.smembers("cart");
carts.splice(0,1);  
Lockr.set("cart",carts);

a nice Lockr.srempos("cart",0) would be nice...

tsironis commented 8 years ago

Cool idea! Let's name it Lockr.srempos, or maybe Lockr.sremat which I think is more concise. What do you think?

hungcaoduy commented 8 years ago

Lockr.srempos or Lockr.sremat would be nice!

If you can go further, how about something like Lockr.srem("cart", {id: 123}) ? an optional option object may help to search and remove matched item/items from a collection. does that make sense?

tsironis commented 8 years ago

Just clarifying Lockr.srem works like _.without though, so if you provide a JSON object, srem will remove all instances containing that key-value pair. Is that the behaviour you would expect?

hungcaoduy commented 8 years ago

I understand the example when the item to add is primitive data like below: Lockr.sadd("wat", 1); Lockr.sadd("wat", 2); [1, 2] Lockr.srem("wat", 1); //[2]

How to do if the Lockr.sadd does not work on primitive data as 1, 2 but object like {id: 123, name: abc}? At a state when Lockr.smembers('cart') get [{id: 123, name: abc}, {id: 456, name: def}], I expect that Lockr.srem('cart', {id: 123}) would remove the first object and Lockr.smembers('cart') would result [{id: 456, name: def}] but it does not work

tsironis commented 8 years ago

Indeed, it doesn't work currently for objects like this yet. I was referring to the existing functionality of srem that removes all instances of a value in a specified array. The proposed changes will have to work in a similar manner, removing all objects that contain instances of key-value pair in specified array.

avgerin0s commented 8 years ago

@hungcaoduy @tsironis True, I confirm this as a bug.

The reason behind this is that indexOf, used by sismember which in turn is used by srem doesn't work on array of Objects.

I'll craft a patch tomorrow as well as add some regression tests.

Thanks for reporting!

PS: As for srempos, I'd prefer to not break the Redis-like API.

tsironis commented 8 years ago

@eavgerinos what we would be a Redis-like method name for srempos? Is there an equivalent?

tsironis commented 8 years ago

ping @eavgerinos

avgerin0s commented 8 years ago

@tsironis http://redis.io/commands#set nope AFAIK.

IMHO, we should fix the behavior of srem in case of objects instead of adding an srempos

ai3gtmc commented 8 years ago

I just ran into this dilemma myself. Anyone ever got it fixed?

tsironis commented 8 years ago

This is not fixed yet. Temporary solution is the one mentioned by @choval though.

sandzOfTime commented 6 years ago

I take it this issue hasn't been resolved as yet.

santiagochen commented 6 years ago

any incoming good news for this issue?

ngrecco commented 6 years ago

Hello everyone. Here's a working implementation from srempos. You can use the index that matches the smembers array.

Lockr.srem = function(key, value, options, positionToRemove) 
{
    var query_key = this._getPrefixedKey(key, options),  json,index;
    var values = Lockr.smembers(key, value);

    if (positionToRemove !== undefined)
    {
        index = positionToRemove;
    }
    else index = values.indexOf(value);

    if ((index > -1) && (index < values.length))
      values.splice(index, 1);

    json = JSON.stringify({"data": values});

    try {
      localStorage.setItem(query_key, json);
    } catch (e) {
      if (console) console.warn("Lockr couldn't remove the "+ value +" from the set "+ key);
    }
  };

    Lockr.srempos = function(key, pos) {
      return Lockr.srem(key, undefined, undefined, pos)
    };