Need to clone objects before storing in storage object #20

Closed
odedbd opened this Issue Nov 17, 2011 · 4 comments

Comments

Projects
None yet
2 participants
@odedbd

odedbd commented Nov 17, 2011

Hi,

First let me thank you for the wonderful little tool you've created, very useful!

I have run into a minor issue, which can be easily fixed (I'm no git user, so I didn't create a pull req for this).

in set:

    set: function(key, value){
        _checkKey(key);
        if(_XMLService.isXML(value)){
            value = {_is_xml:true,xml:_XMLService.encode(value)};
        }
        _storage[key] = value;
        _save();
        return value;
    }

If the value is an object, and it is later changed, the _storage cache will reflect those changes (while the localStorage won't). This is an issue, since the stored object may (and in my application does) acquire non-json-serializable properties (such as jQuery wrapped elements). when ever some other key is changed and the entire _storage object needs to be serialized, things break.

my fix was quite simple, perhaps there's a better way to go about this-

set: function(key, value){
        var clone;
        _checkKey(key);
        if(typeof(value) === 'object'){
            clone = $.extend(true, {}, value);
        }else{
            clone=value;
        }
        if(_XMLService.isXML(clone)){
            clone= {_is_xml:true,xml:_XMLService.encode(clone)};
        }
        _storage[key] = clone;
        _save();
        return clone;
}

Please note that this uses jQuery's extend function explicitly, I have no idea whether this could roll with other libraries. This works fine for me, so I hope it might help others as well.

Thanks again,
o

@andris9

This comment has been minimized.

Show comment
Hide comment
@andris9

andris9 Nov 21, 2011

Owner

Hi,

As jStorage supports many different libraries besides jQuery (Prototype, MooTools or any other with JSON support) then using jQuery specific methods is not acceptable.

There are some issues that can't be solved by the library. For example when an object includes a DOM node as a property, then JSON serialization in Chrome breaks big time. It is not feasible for jStorage to walk the entire node tree of an object to check if its properties can be saved or not, as it indroduces severe overhead for larger objects. Then there's circular structures etc. Adding such complexity to jStorage isn't probably the best thing to do.

Owner

andris9 commented Nov 21, 2011

Hi,

As jStorage supports many different libraries besides jQuery (Prototype, MooTools or any other with JSON support) then using jQuery specific methods is not acceptable.

There are some issues that can't be solved by the library. For example when an object includes a DOM node as a property, then JSON serialization in Chrome breaks big time. It is not feasible for jStorage to walk the entire node tree of an object to check if its properties can be saved or not, as it indroduces severe overhead for larger objects. Then there's circular structures etc. Adding such complexity to jStorage isn't probably the best thing to do.

@andris9 andris9 closed this Nov 21, 2011

@odedbd

This comment has been minimized.

Show comment
Hide comment
@odedbd

odedbd Nov 21, 2011

Hi Andris,

I totally understand about the jQuery specific method, as I have written in my issue.

The main point here is not so much serialization of methods/nodes. In fact, it isn't event related to working with localStorage, strictly speaking. It's simply the way the internal object _storage keeps, for object, references to the actual objects passed to the function. this is a problem, since this may lead to mismatches between what's in the localStorage and what's in the _storage object. In case that the object passed to set has changed and no set was called yet, _storage will reflect this change, while localStorage would not.

I understand why you consider that this cannot be solved in the manner I did within the library, but I would like to suggest two other options that might better suite:

  1. just document this issue, and make it clear to users of the library that they are responsible in cloning (or not changing) objects that they set as values in order to prevent this. this is a non-trivial bug to debug, since it usually comes back to bite you only when you are trying to set some other key, which may not even be an object, but just triggers a save, which tries to serialize a previously set object which changed after setting, and had non jsonable properties added to it.

  2. it is also possible to simply not save objects into the _storage cache in the first place, so that they are always read from the localStorage. this can have serious performance costs, so I guess it is not preferable.

As for myself, I intend to wrap up the set function with some facade that will implement this logic (using jquery's extend), since it will solve my specific issue. I think that my option 1) here is a good idea, and I hope you might agree, for the sake of future users of the library.

Thanks again for this excellent piece of code that helps make my life as a developer a little easier :)

odedbd commented Nov 21, 2011

Hi Andris,

I totally understand about the jQuery specific method, as I have written in my issue.

The main point here is not so much serialization of methods/nodes. In fact, it isn't event related to working with localStorage, strictly speaking. It's simply the way the internal object _storage keeps, for object, references to the actual objects passed to the function. this is a problem, since this may lead to mismatches between what's in the localStorage and what's in the _storage object. In case that the object passed to set has changed and no set was called yet, _storage will reflect this change, while localStorage would not.

I understand why you consider that this cannot be solved in the manner I did within the library, but I would like to suggest two other options that might better suite:

  1. just document this issue, and make it clear to users of the library that they are responsible in cloning (or not changing) objects that they set as values in order to prevent this. this is a non-trivial bug to debug, since it usually comes back to bite you only when you are trying to set some other key, which may not even be an object, but just triggers a save, which tries to serialize a previously set object which changed after setting, and had non jsonable properties added to it.

  2. it is also possible to simply not save objects into the _storage cache in the first place, so that they are always read from the localStorage. this can have serious performance costs, so I guess it is not preferable.

As for myself, I intend to wrap up the set function with some facade that will implement this logic (using jquery's extend), since it will solve my specific issue. I think that my option 1) here is a good idea, and I hope you might agree, for the sake of future users of the library.

Thanks again for this excellent piece of code that helps make my life as a developer a little easier :)

@andris9

This comment has been minimized.

Show comment
Hide comment
@andris9

andris9 Nov 23, 2011

Owner

I think I came up with a solution which satisfies everybody. The value is cloned on save by de/jsonizing it. This also fixes the issue of function values and object methods in saved data being accessible after save but not after page refresh. a84c1aa

Owner

andris9 commented Nov 23, 2011

I think I came up with a solution which satisfies everybody. The value is cloned on save by de/jsonizing it. This also fixes the issue of function values and object methods in saved data being accessible after save but not after page refresh. a84c1aa

@odedbd

This comment has been minimized.

Show comment
Hide comment
@odedbd

odedbd Nov 28, 2011

This sounds like a great idea. I will let you know if I run into any trouble with it.

Thanks. for your work and for your patience.

odedbd commented Nov 28, 2011

This sounds like a great idea. I will let you know if I run into any trouble with it.

Thanks. for your work and for your patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment