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

Caching: handling 304 responses #13

Open
gr2m opened this issue Dec 25, 2017 · 6 comments
Open

Caching: handling 304 responses #13

gr2m opened this issue Dec 25, 2017 · 6 comments

Comments

@gr2m
Copy link
Contributor

gr2m commented Dec 25, 2017

This is a follow up for octokit/octokit.js#673 (comment) /cc @jsg2021

The documentation on Conditional requests says

Most responses return an ETag header. Many responses also return a Last-Modified header. You can use the values of these headers to make subsequent requests to those resources using the If-None-Match and If-Modified-Since headers, respectively. If the resource has not changed, the server will return a 304 Not Modified.

Maybe more importantly for us, it says

Note: Making a conditional request and receiving a 304 response does not count against your Rate Limit, so we encourage you to use it whenever possible.

(this should probably be added to the documentation on Best practices for integrators?)

My question is: do you / should we handle caching as part of the Octokit libraries?

I would say yes, the same way there should be APIs for pagination or handling of rate limits. I’m curious what we currently do in the other implementations

@jsg2021
Copy link

jsg2021 commented Dec 27, 2017

If you add transparent handling of caching, you should not preclude consumers opting out and handling it themselves... or provide a cache-provider plug-in api (so consumers can serialize/persist the cache in whatever form they want... be it memcache, json files, etc)

@gr2m
Copy link
Contributor Author

gr2m commented Dec 27, 2017

My current idea is to make an official plugin that people can use, but can replace with their own. But I’m curious to hear what maintainers of octokit.net & octokit.rb have worked out

@M-Zuber
Copy link

M-Zuber commented Dec 27, 2017

Not a maintainer but I found some links related to octokit.net that might be useful

@ryangribble
Copy link
Contributor

Here is another octokit.net discussion about my thoughts on implementing ETag/caching support.

octokit/octokit.net#1636 (comment)

From my perspective I prefer the idea of exposing the necessary information to consumers (eg a way for them to know what the ETag of a response was, and a way to provide that ETag or a timestamp on a call) and let them use them as they see fit, as opposed to "magic" internal workings that automatically cache responses and use ETags internally without the user knowing about it etc.

I also prefer the idea of returning a blank collection when the 304 response is received, rather than returning a cached response - again because I feel like octokit is an API wrapper and not an actual application layer itself

@kytrinyx
Copy link

💯 to providing helper methods to make it easy to do pagination, caching, and handling rate limit responses, etc. I'd rather not do it transparently, for all the reasons @ryangribble mentions.

@Disturbing
Copy link

Would love to see this feature implemented! Exposing this in ApiOptions alone should be the way. Agreed that it should be an Api Wrapper, not an Application Layer.

Common features that sit on top of OctoKit.Net such as adding extension methods that cache this stuff in appointed repos is how I'd see this going.

oo-bldrs added a commit to oo-bldrs/Share that referenced this issue Nov 14, 2022
pablo-mayrgundter pushed a commit to bldrs-ai/Share that referenced this issue Feb 6, 2023
* Add Auth0 React SDK

* Activate Auth0 context (with values from environment)

* Make use of refresh tokens

* Stop stripping query parameters off of the URL upon redirect

* Initial elements to allow for login and display avatar

* Use Github icon for login button

* Confirm non-null state before dereferencing

* Add context menu to display name and logout button

* Change logout to roll to configured URL or fallback to window origin

* Use Octokit mock only when we're in a testing environment

* Update yarn lockfile

* Default arguments to empty object

* Add Accept: header for Github API calls

* Add HTTP request header for authorization

* Use access token to make request for comments

* Do not raise a popup for retrieving Auth0 access token

* Send access token on remote call

TODO: Refactor into single function to avoid eliminate duplication

* Fix arguments passed to Octokit

* Allow GitHub base URL to be set via an environment variable

* No-op commit

* Remove commented out code (Octokit instantiation)

* Update esbuild configuration to pass along null if no redirect URI provided

* Update size of GitHub avatar to be in line with new UI

* Update dependency lock file

* Correct capitalization of GitHub

* Create store member for GitHub access token

* Use access token from internal store

* Retrieve access token upon application load (if available)

* Remove debug console logging

* No need to destructure Zustand state value

* Conditionally set authorization when requesting IFC

* Add missing useEffect dependency

* New function to parse GitHub repository URLs

* Do not prepend leading slash when returning path

* New function to get object direct download URL

* Ensure exceptions are bubbled up from Octokit

* Use boolean variable to reflect whether this is a new file

* Add utility functions to get target IFC URL

* Perform IFC URL "pre-resolution" only if we're logged in

* Let downstream handle mapping of target GitHub host

* Move repository URL resolution functions outside of component

* Remove access token comparison from conditional

* Add escape hatch for IFCs hosted in this repository

* Add missing hook dependency for access token

* Add missing argument in child function

* Don't attempt to load viewer why authentication is happening

* Do not allow cached responses for download URL retrieval

Related to octokit/discussions#13

* Replace existing path and reload page upon Auth0 login

* Add existing path for login redirect

* Fix ESLint errors

* Replace ESLint rule suppression with newlines

* Reintroduce URL transform logic

* Migrate to v2 provider syntax

* Change Auth0 scope request to offline access

* Disable refresh tokens (temporarily)

* Remove duplicate Share button introduced by merge

* Use leading slash on file paths in tests

* Fix inconsistent labeling and title of login button

* Create set of mocks for authentication-based rendering

* Add unit tests for rendering the authentication navigation component

* Replace external image with inlined data
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

No branches or pull requests

6 participants