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

Make HTTP errors cacheable #4

Merged
merged 3 commits into from Dec 17, 2015
Merged

Make HTTP errors cacheable #4

merged 3 commits into from Dec 17, 2015

Conversation

robinjmurphy
Copy link
Contributor

  • HTTP errors (404, 500 etc.) are now cached as indicated by the cache-control header
  • This relies on a change to our underlying cache data format
  • We no longer just store the body in the cache
  • We now store an object with either a body property or an error property
  • The error property contains everything we need to recreate an Error object when we pull it out of the cache
  • err.message, err.statusCode, err.body
  • The above is required as we can't just JSON stringify Error objects

A valid response is cached as:

{
  "body": {
    "foo": "bar"
  }
}

A HTTP error is cached as:

{
  "error": {
    "message": "An error occurred",
    "statusCode": 503,
    "body": {
      "oh": "no!"
    }
  }
}

* Changes the underlying cached format
* Now caches an object, not just the response body
* This is to make way for caching the error object
* Stores all of the properties of HTTP errors in the cache
* Message, statusCode, body
* Recreates error objects when pulling them out of the cache
* We can't store Error objects in JSON so we need to recreate them
statusCode: err.statusCode,
body: err.body
}
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're storing everything we need to recreate a proper Error object.

@robinjmurphy
Copy link
Contributor Author

Would be interested in your feedback @nspragg @hjerling @DaMouse404.

@robinjmurphy
Copy link
Contributor Author

Note that we're free to change the cached object structure in any release because the cache key includes the version number of the library.

@hjerling
Copy link
Contributor

👍
Looks good to me. I had a bit of a dig through to make sure that we don't use the cached error on retries but that is clearly out of harms way.
I like the way it caches any response based on cache-control, regardless of success or error!
🍪

@Mousius
Copy link
Contributor

Mousius commented Dec 16, 2015

Looks like a good idea! 👍

@nspragg
Copy link
Contributor

nspragg commented Dec 17, 2015

👍

robinjmurphy added a commit that referenced this pull request Dec 17, 2015
@robinjmurphy robinjmurphy merged commit a59c740 into master Dec 17, 2015
@robinjmurphy robinjmurphy deleted the make-errors-cacheable branch December 17, 2015 10:42
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

4 participants