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

Add caching for custom markers form URLs #177

Merged
merged 3 commits into from
Aug 9, 2024
Merged

Conversation

kochis
Copy link
Collaborator

@kochis kochis commented Aug 1, 2024

  • Cache custom markers to avoid unnecessary requests

Before

Network request for every marker image

markers-no-cache.mov

After

Single request for each marker URL used

markers-with-cache.mov

@@ -29,6 +29,29 @@ interface ImageOptions {
height?: number | string;
}

// cache URL loaded markers
const IMAGE_CACHE = new Map<string, Blob | string>();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TODO: use enum here instead of string

const interval = setInterval(() => {
const cachedData = IMAGE_CACHE.get(url);
if (cachedData === 'pending') {
if (Date.now() - start > timeoutMS) { // expired
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

expired -> timed out

Comment on lines +152 to +158
useCachedImage(markerOptions.url)
.then(onSuccess)
.catch((reason: 'miss' | 'timedout' | 'failed' | Error) => {
if (reason !== 'miss') {
Logger.debug(`RadarMarker: cache lookup for ${markerOptions.url}: ${reason}`);
}
})
.catch(onError)
loadImage();
Copy link
Contributor

Choose a reason for hiding this comment

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

might be nice to just have useCachedImage do the 1) check if in cache 2) if its not, load it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only think that makes it sorta tricky is the image loading happens differently depending on the type of marker being loaded (either from a remote URL, or via authenticated request to Radar). Can maybe play around with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

yea maybe could pass in a loadImage function, but not a huge deal

@kochis kochis merged commit 481dabf into master Aug 9, 2024
4 checks passed
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.

2 participants