Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Watch for changes in local storage #166

Closed
DaveWM opened this issue Nov 6, 2014 · 11 comments · May be fixed by #167
Closed

Watch for changes in local storage #166

DaveWM opened this issue Nov 6, 2014 · 11 comments · May be fixed by #167

Comments

@DaveWM
Copy link

DaveWM commented Nov 6, 2014

When you call localStorageService.bind, it would be nice if it then listened for changes to the local storage key it binds to, and updated the bound object (as suggested by @lieryan in #131). This can be done by registering an event listener for the "storage" event. PR incoming.

@YouriT
Copy link

YouriT commented Nov 26, 2014

Because of the serialisation of the objects, I think it may be useful to have an event or a callback to be able to detect that the binding has been done is from LS to the scope.

I explain myself:
Let's say I've this:

{
    a: 'a',
    b: 'b',
    myFn: function() {
        console.log('test');
    }
}

After the serialisation, it will be:

"{"a":"a","b":"b"}"

So I've lost myFn of the scope which is normal. So if we have an event we can rebind that to the $scope variable and avoid the call to undefined function...

What do you think about that?

@YouriT
Copy link

YouriT commented Nov 26, 2014

Or we could use : http://www.eslinstructor.net/jsonfn/ to avoid this issue ? Don't know what's better.. Maybe this one

@DaveWM
Copy link
Author

DaveWM commented Nov 26, 2014

@YouriT Personally, I don't like the idea of broadcasting an event - you have to know to listen for the event (which is not immediately obvious), it may not be possible to simply re-bind the function, the event name is a magic string, etc.

I'm not sure it would be possible to serialize functions, other than extremely simple ones (that library you linked to just calls .toString() on the function).

I'd say you shouldn't have any unserializable properties on the bound object in the first place. Anyone else want to weigh in on this?

@YouriT
Copy link

YouriT commented Nov 26, 2014

I agree on the serialisation of functions may be tricky with complex one, that's why I opted for the event broadcasting.

Why shouldn't I have that ? On my side it perfectly makes sense because it's context dependant and I don't want to each time inject the data to a function that will then do things in an appropriate form.

If you don't like the idea of the broadcasted event, it could be a callback on the bind function and then it would be maybe more straightforward don't you think @DaveWM ?

@DaveWM
Copy link
Author

DaveWM commented Nov 27, 2014

I've updated my PR, when local storage is updated it now extends the bound object rather than overwrites it. This should preserve any unserializable properties, and saves you having to wire up events all over the place. How does that sound to you @YouriT ?

@YouriT
Copy link

YouriT commented Nov 27, 2014

Sounds quite good. Didn't think about that. Thanks !

@lieryan
Copy link

lieryan commented Nov 28, 2014

What would happen to deleted properties?

@DaveWM
Copy link
Author

DaveWM commented Dec 2, 2014

If you delete a property of the object in local storage, it won't be removed from the javascript object. I can see how this could be confusing, anyone else have any thoughts on this?

@YouriT
Copy link

YouriT commented Dec 2, 2014

Maybe some markup could help ? I changed anyhow the way it works on my side because it's hard to keep consistency across different pages... I would rather go just for a proper event listening like an additional parameter to the bind function.

Something else, I saw that you used addEventListener, don't you wanna use a more cross browser function ? Like this one:

function addEventListener(obj, evt, fnc) {
    // W3C model
    if (obj.addEventListener) {
        obj.addEventListener(evt, fnc, false);
        return true;
    }
    // Microsoft model
    else if (obj.attachEvent) {
        return obj.attachEvent('on' + evt, fnc);
    }
    // Browser don't support W3C or MSFT model, go on with traditional
    else {
        evt = 'on' + evt;
        if (typeof obj[evt] === 'function') {
            // Object already has a function on traditional
            // Let's wrap it with our own function inside another function
            fnc = (function(f1, f2) {
                return function() {
                    f1.apply(this, arguments);
                    f2.apply(this, arguments);
                };
            })(obj[evt], fnc);
        }
        obj[evt] = fnc;
        return true;
    }
    return false;
};

@DaveWM
Copy link
Author

DaveWM commented Dec 2, 2014

@YouriT Listening for events isn't really the "angular way", you should be able to rely on the binding alone. What exactly are you trying to "keep consistency" of?

addEventListener is implemented in IE9, so I don't think we need to worry about this.

@eddiemonge
Copy link
Collaborator

duplicate of #84

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

Successfully merging a pull request may close this issue.

5 participants