-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: add cache buster #243
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #243 +/- ##
=======================================
Coverage 99.94% 99.94%
=======================================
Files 52 52
Lines 1938 1946 +8
=======================================
+ Hits 1937 1945 +8
Misses 1 1 ☔ View full report in Codecov by Sentry. |
src/extension/content.js
Outdated
/** | ||
* Removes the cache buster from the URL. | ||
*/ | ||
function removeCacheParam() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't love this defined here but didn't think it fit into any other util files and didn't want to create another one (to keep the number of files the sidekick loads to a minimum). Do you see this fitting anywhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't particularly mind having this here, but maybe tab.js
would be a better fit. We could call it from checkTab()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since removeCacheParam()
uses window
we can't call it from checkTab
unfortunately. Since you don't mind, I'm thinking I will just keep it where it is.
@@ -962,15 +962,6 @@ export class AppStore { | |||
// update live | |||
const resp = await this.api.updateLive(path); | |||
if (resp) { | |||
// bust client cache for live and production |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe any of this was doing anything unless it just so happened that the publish happened same origin. This logic is instead moved to the publish plugin.
# [1.31.0](v1.30.2...v1.31.0) (2024-09-09) ### Features * add cache buster ([#243](#243)) ([63a7dc5](63a7dc5))
🎉 This PR is included in version 1.31.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
fixes: #216
Experience should be
Gdrive/sharepoint -> preview (cacheBust)
Reload on preview (no cache bust.. instead cache: reload
Publish on preview (cacheBust)
Publish on publish (no cache bust.. instead cache: reload