-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add option to support caching #23
Comments
This is a good idea for node.js but I don't recommend it for the browser. You'll never cache more efficiently than the browser does. If you add caching as an option then I would try to make it very explicit in the docs that it's not recommended for browsers. |
I'm not sure the browser is caching any responses at present. There are no cache-related headers in the responses to tell the browser to cache the responses, and I don't think there can be, since the server can't know in advance how long the browser could cache a response. The only way I can see to cache in the browser or in a node.js client is for the SDK to cache the responses along with their etags, and to send the If-None-Match header on subsequent GET requests to the same URL. Am I missing something? |
I get 304s when I query the API with just jQuery in the browser, and I get them too when using the widget Dovy's been building: http://rootsdev.org/SumoApp/profile.html. Do you not get 304s? |
All I did was create a variable and on each call, I check if the variable has the person already populated. If it does, then we're good. If not, it does a person call. Pretty simple, but effective. |
You're right - I'd disabled caching in my browser, which is why I wasn't seeing the 304s. Once I enabled it, everything worked. :-) There's no need for additional caching on the browser. I won't close this issue because it could be useful at the server, but it won't be implemented until I add node.js support. |
I've changed my views on this. FS has some caching headers but persons (and maybe relationships) are the only objects with really effective caching. I would like to see a user-defined/configurable cache. In other words, the SDK defines the API and the user passes in an object to the SDK which implements that API. That would allow for an in-memory cache during testing, maybe a localStorage cache in the browser, maybe a redis cache on the server, etc. |
Something like this: https://github.com/godaddy/node-http-cache |
Add an option to turn on caching to the init call. If turned on, the SDK should cache GET responses along with their etags, and make conditional GET requests when clients request data that has been cached. See https://familysearch.org/developers/docs/api/tree/Read_Not-Modified_Person_usecase for an example.
Consider depending on https://github.com/isaacs/node-lru-cache for the cache - it works in both browser and node environments. SDK users would need to include this library if caching is turned on.
The text was updated successfully, but these errors were encountered: