Fix private browsing issue #44

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants
@andriijas

This comment has been minimized.

Show comment
Hide comment
@andriijas

andriijas Oct 29, 2012

Contributor

Nice but feels like this generalizes a bit to much.

Contributor

andriijas commented Oct 29, 2012

Nice but feels like this generalizes a bit to much.

@jeromegn

This comment has been minimized.

Show comment
Hide comment
@jeromegn

jeromegn Oct 29, 2012

Owner

How about calling options.error with the error when that error is caught?

Also, would you mind minifying it? I left instructions on how to do so here: http://documentup.com/jeromegn/backbone.localstorage#contributing

Ideally, you'd create a test for this, basically stubbing localStorage with something that throws the error (temporarily) and test if it's caught and sent back the right way. I can see how this might be a bit much though ;)

Owner

jeromegn commented Oct 29, 2012

How about calling options.error with the error when that error is caught?

Also, would you mind minifying it? I left instructions on how to do so here: http://documentup.com/jeromegn/backbone.localstorage#contributing

Ideally, you'd create a test for this, basically stubbing localStorage with something that throws the error (temporarily) and test if it's caught and sent back the right way. I can see how this might be a bit much though ;)

@gsmaverick

This comment has been minimized.

Show comment
Hide comment
@gsmaverick

gsmaverick Oct 29, 2012

What's wrong with the solution proposed here: marcuswestin/store.js#42 (comment)?

I think the author's comments there apply in this case as well. It should not be the library's responsibility to detect this issue. Your fix hides the actual issue (localStorage isn't available) and so instead it appears as though it's just a regular error.

What's wrong with the solution proposed here: marcuswestin/store.js#42 (comment)?

I think the author's comments there apply in this case as well. It should not be the library's responsibility to detect this issue. Your fix hides the actual issue (localStorage isn't available) and so instead it appears as though it's just a regular error.

@sebastien-p

This comment has been minimized.

Show comment
Hide comment
@sebastien-p

sebastien-p Oct 29, 2012

That was just a quick fix that we can improve for sure.

@jeromegn Yes, calling option.error passing the actual error is better. I'll try to complete this pull request when I have time.

@gsmaverick Correct me if I'm wrong but the original Backbone.sync implementation will call options.error whenever a model failed to save on the server. I think this pretty much emulates this kind of behavior.

That was just a quick fix that we can improve for sure.

@jeromegn Yes, calling option.error passing the actual error is better. I'll try to complete this pull request when I have time.

@gsmaverick Correct me if I'm wrong but the original Backbone.sync implementation will call options.error whenever a model failed to save on the server. I think this pretty much emulates this kind of behavior.

@sebastien-p

This comment has been minimized.

Show comment
Hide comment
@sebastien-p

sebastien-p Nov 4, 2012

@jeromegn I'm not sure how to write a test which would cover this edge case.

@jeromegn I'm not sure how to write a test which would cover this edge case.

@jeromegn

This comment has been minimized.

Show comment
Hide comment
@jeromegn

jeromegn Jan 12, 2013

Owner

Are we letting this go? Is this still an issue in more recent browser versions?

Here's how a test would work: override window.localStorage temporarily to return the same value a browser in private browsing mode would return.

Owner

jeromegn commented Jan 12, 2013

Are we letting this go? Is this still an issue in more recent browser versions?

Here's how a test would work: override window.localStorage temporarily to return the same value a browser in private browsing mode would return.

@jeromegn jeromegn closed this in 93ddfff Jan 13, 2013

@sebastien-p

This comment has been minimized.

Show comment
Hide comment
@sebastien-p

sebastien-p Jan 14, 2013

@jeromegn thanks a lot! Now I see how I should have done this, sorry.

@jeromegn thanks a lot! Now I see how I should have done this, sorry.

@jeromegn

This comment has been minimized.

Show comment
Hide comment
@jeromegn

jeromegn Jan 14, 2013

Owner

My pleasure.

Owner

jeromegn commented Jan 14, 2013

My pleasure.

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