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

(chore): ajax cache to reduce server load #872

Merged
merged 2 commits into from
Oct 24, 2023
Merged

Conversation

ndbiaw
Copy link
Contributor

@ndbiaw ndbiaw commented Oct 19, 2023

normal, ajax will add timestamp at the end of request. this config remove it

@Difegue
Copy link
Owner

Difegue commented Oct 20, 2023

👋 Wouldn't this lead to users not getting up-to-date data when they modify metadata for their archives, which changes the internal indexes/the result of the search API call?

There's already serverside cache for searches, so I don't think this would be very useful 🤔

@ndbiaw
Copy link
Contributor Author

ndbiaw commented Oct 22, 2023

👋 Wouldn't this lead to users not getting up-to-date data when they modify metadata for their archives, which changes the internal indexes/the result of the search API call?

cache: true doesn't actually change anything, cache: false just adds a timestamp at the end of the URL to prevent content from being cached if the appropriate header is present. User can still get up-to-date results, this only helps if the user uses the header to set the cache but ajax always adds timestamp to the end of the URL so making browser cache useless. This is useful for reducing the request to the server.

There's already serverside cache for searches, so I don't think this would be very useful 🤔

This change reducing the request to the server if the user sets the appropriate header, so I don't think server-side caching is really relevant, because if the request is cached it won't make the request to the server.

@Difegue Difegue merged commit 9a0d90d into Difegue:dev Oct 24, 2023
@Difegue
Copy link
Owner

Difegue commented Oct 24, 2023

Thanks for the explanation - Don't see anything wrong with merging this in that case. Thanks!

@holopin-bot
Copy link

holopin-bot bot commented Oct 24, 2023

Congratulations @ndbiaw, you just earned a holobyte! Here it is: https://holopin.io/holobyte/clo4jpati130420flb5ayoq2sn

This badge can only be claimed by you, so make sure that your GitHub account is linked to your Holopin account. You can manage those preferences here: https://holopin.io/account.
Or if you're new to Holopin, you can simply sign up with GitHub, which will do the trick!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants