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

Revert disk cache store #1062

Merged
merged 5 commits into from
Nov 13, 2023
Merged

Revert disk cache store #1062

merged 5 commits into from
Nov 13, 2023

Conversation

afcapel
Copy link
Collaborator

@afcapel afcapel commented Nov 13, 2023

This reverts #949 and the follow up PRs #1014 and #1056.

The idea is still appealing but we've found several hard-to-debug issues while testing this in our applications. Most notably we found some meta tags propagating outside the original page on navigation, and issues with the timing of turbo:load events.

These issues are solvable, but our priority right now is shipping Page Refreshes ahead of Turbo v8.

Copy link
Collaborator

@kevinmcconnell kevinmcconnell left a comment

Choose a reason for hiding this comment

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

Looks good @afcapel.

It's a shame if the disk cache doesn't end up making it into v8, but I agree with you about the priorities so it's a good call to take it back out for now.

@@ -62,11 +62,11 @@ setup(() => {
Turbo.registerAdapter(adapter)
})

test("navigator adapter is native adapter", async () => {
test("test navigator adapter is native adapter", async () => {
Copy link
Collaborator

@kevinmcconnell kevinmcconnell Nov 13, 2023

Choose a reason for hiding this comment

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

I think the extraneous test in the descriptions were removed after the original changes were added, and so reverting is bringing some of them back. Might we worth removing them again as an additional commit in this PR, for consistency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, I've added the commits to the branch.

The branch was gone, so I had to recreate the commits. Definitely one case in which squashing the branch was not very helpful.

@afcapel afcapel merged commit 675d626 into main Nov 13, 2023
2 checks passed
@afcapel afcapel deleted the revert-disk-cache branch November 13, 2023 16:47
afcapel added a commit that referenced this pull request Nov 14, 2023
We refactored the cache class to not take a session in the constructor
in 485115d.

But after reverting the disk cache #1062
the cache needs a reference to the session again.
afcapel pushed a commit that referenced this pull request Nov 14, 2023
We refactored the cache class to not take a session in the constructor
in 485115d.

But after reverting the disk cache #1062
the cache needs a reference to the session again.
afcapel added a commit that referenced this pull request Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants