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

Clear cache when historyCacheSize is set to 0 #1222

Merged
merged 2 commits into from Oct 26, 2023

Conversation

aomader
Copy link
Contributor

@aomader aomader commented Jan 26, 2023

Fixes #1221

@Telroshan
Copy link
Collaborator

Hey, if you're still interested in merging those changes, could you please retarget your PR to the dev branch ?

@aomader aomader changed the base branch from master to dev September 25, 2023 11:55
@aomader
Copy link
Contributor Author

aomader commented Sep 25, 2023

@Telroshan Here you go.

@Telroshan Telroshan added the ready for review Issues that are ready to be considered for merging label Sep 25, 2023
@alexpetros alexpetros changed the title fix(history): clear cache in case historyCacheSize is set to 0 Clear cache when historyCacheSize is set to 0 Sep 26, 2023
src/htmx.js Outdated
@@ -2193,6 +2193,7 @@ return (function () {
historyCache.shift(); // shrink the cache and retry
}
}
if (historyCache.length === 0) localStorage.removeItem("htmx-history-cache");
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment here?

Also maybe this should be up near the top and avoid all the logic in this method?

Copy link
Contributor

Choose a reason for hiding this comment

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

@aomader can you check in on this? Would like to merge this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1cg I added a comment and moved it to the top to exit early. Note however that this changes the current behavior w.r.t. the htmx:historyItemCreated that was triggered up until now. However, I'd argue that the new behavior is more correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree

@1cg 1cg merged commit ff45076 into bigskysoftware:dev Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready for review Issues that are ready to be considered for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

History cache not cleared after historyCacheSize has been set to 0 and cache was not empty
3 participants