Task: Add browser storage for resources #2373
Conversation
…ph-explorer-v4 into fix/in-browser-caching-resources
MaryannGitonga
requested review from
thewahome,
darrelmiller,
ElinorW and
Onokaev
MaryannGitonga
changed the title
thewahome
changed the title
|
We should remember to remove the local json files |
| const response = await fetch(resourcesUrl, options); | ||
| if (response.ok) { | ||
| const resources = await response.json() as IResource; | ||
| return dispatch(fetchResourcesSuccess(resources)); | ||
| } | ||
| throw response; | ||
| } catch (error) { | ||
| const cachedResources = await resourcesCache.readResources(); |
There was a problem hiding this comment.
IMO we should be using the cached data in preference to hitting the DevX API every time, it will be much quicker.
There was a problem hiding this comment.
We only call the api once when the page loads. And there are no use cases where for the app to work a reload is required.
I'm not sure that there'd be a use case for the preference
There was a problem hiding this comment.
Our users never hit F5? or visit multiple times a day?
There was a problem hiding this comment.
that could happen.
To prefer cache, we'd need to make it able to expire so that we can sometimes get the latest.
There was a problem hiding this comment.
Yeah, we would need to also store a refresh interval, note, this not an expiry as we wouldn't remove anything from the cache, just have a point in time that we'd try to update the content of the cache after.
That said, I've been thinking on this a bit and chatting with @darrelmiller
We should be looking to ensure that the DevX API provides appropriate cache control headers and not worrying about adding a complex caching mechanism here.
Honestly the less code we write the better in terms of cost of ownership, and it allows us to focus on other areas.
Do we know how frequently the data in the DevX API is updated?
There was a problem hiding this comment.
We had a chat with @darrelmiller and we uncovered something I consider a large spanner in the works. The cache control headers do not work in instances where the client and the api are not in the same domain.
An alternative exists that we use service workers to try this. However I am not sure whether the code changes will be any less than the minimal work we have done now.
There was a problem hiding this comment.
@gavinbarron, @darrelmiller I am more inclined to :
- following your suggesion to prefer cache.
- coming up with a mechanism to expire cache. For resources, we could take advantage of knowledge that the openapi document is refreshed every week. Or simply set cache to expire after 7 days
Tell me what you think.
There was a problem hiding this comment.
The cache control headers do not work in instances where the client and the api are not in the same domain.
O_o
I'm really surprised to hear this because this is literally the situation for resources served from a CDN, those are always cross domain and cache in the browser based on cache control headers.
Is there a way I can take a look at what you're seeing to validate this?
There was a problem hiding this comment.
@darrelmiller found this when searching. It was a shared screen where we were able to see the results of the query together.
I've been looking this up and it looks like it could be possible even when the client and the API are not in the same domain as long as the client origin is allowed in the API.
There was a problem hiding this comment.
Yeah, we should be able to set the allowed headers in the response to the options calls from the server:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Headers
| @@ -183,7 +184,7 @@ const UnstyledResourceExplorer = (props: any) => { | |||
|
|
|||
| return ( | |||
| <section style={{ marginTop: '8px' }}> | |||
| {!isolated && <> | |||
| {!isolated && items[0].links.length > 0 && <> | |||
There was a problem hiding this comment.
This is what the condition is trying to preempt from rendering: the search box and the switch to beta toggle button.
src/app/views/sidebar/resource-explorer/panels/postman.util.spec.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Gavin Barron <[email protected]>
…/microsoftgraph/microsoft-graph-explorer-v4 into fix/in-browser-caching-resources
MaryannGitonga
changed the title
…/microsoftgraph/microsoft-graph-explorer-v4 into fix/in-browser-caching-resources
|
SonarCloud Quality Gate failed. |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-2373.centralus.azurestaticapps.net |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-2373.centralus.azurestaticapps.net |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-2373.centralus.azurestaticapps.net |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-2373.centralus.azurestaticapps.net |
|
Kudos, SonarCloud Quality Gate passed!
|
Overview
Closes #2369
Demo
When the first fetch is unsuccessful, and the storage instance is empty, this is displayed to the user:
Otherwise, the resources are displayed (served from DevX API or the browser storage when DevX API is down).
Notes
Optional. Ancillary topics, caveats, alternative strategies that didn't work out, anything else.
Testing Instructions