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

set timestamp safely on response #11

Closed
wants to merge 2 commits into from

Conversation

treasonx
Copy link

@treasonx treasonx commented Nov 2, 2012

www.gruntjs.com is running into a problem right now in chrome where when trying to set the TTL its throwing an exception due to localStorage being full. It looks like the timestamp should be set in the try catch when the response comes back.

@paulirish
Copy link
Collaborator

Hmmm... i can understand try/catching it but dropping the conditional around that seems like an odd choice.

@treasonx
Copy link
Author

treasonx commented Nov 3, 2012

The conditional for setting the ttl seemed a bit odd to me given what the script is trying to accomplish.

  1. Why do we set the ttl outside the success callback? Shouldn't the ttl be set when the data is stored on success? What if the request fails? Do we really want to set a ttl in a failure case? If we did there would be a ttl with no associated payload, which wastes space.

  2. The only way we get into the block where the conditional was, is if we have no data and we need to retrieve it. The conditional was redundant. The only way we would get into the block where the conditional was is it we had no data. The only way we had no data was if the ttl was null or we had just expired it.

@SaneMethod SaneMethod closed this Apr 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants