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

history.replaceState is not compatible with turbo #234

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

dbil25
Copy link
Contributor

@dbil25 dbil25 commented Mar 29, 2024

history.replaceState is not compatible with turbo. it breaks the page caching (when hitting previous/next)

got the turbo code from here: hotwired/turbo#335

@excid3
Copy link
Owner

excid3 commented Mar 29, 2024

Looks like this has some issues as well?

  • User cannot return to the previous state anymore
  • Preview holds outdated state

hotwired/turbo#335 (comment)

@dbil25
Copy link
Contributor Author

dbil25 commented Mar 29, 2024

Oops i skipped past those apparently 😕. When i tried, it totally fixed it though.

The issue is simple to reproduce, just click a link in a turbo app while in a page that contains this tab component, then pressing previous does not work at all.

I'll dig around next to try and find a perfect solution.

@dbil25
Copy link
Contributor Author

dbil25 commented Mar 30, 2024

I don't understand what pySilver is refering to. It works flawlessly and doesnt break the state. In the discussion he is referring to, he is trying to do a 'pushState' not a 'replaceState', which might be why he said that we "lose the previous state", but in this case its expected. I dont really see a problem with this approach, and when looking at turbo source code, it just does a history.replaceState but with a little additional turbo stuff to keep the cache intact: https://github.com/hotwired/turbo/blob/9fb05e3ed3ebb15fe7b13f52941f25df425e3d15/src/core/drive/history.js#L38

@excid3
Copy link
Owner

excid3 commented Apr 3, 2024

I just need to test this in a Turbo app, but this seems good to me.

@dillonhafer
Copy link

I'm using the tabs.js file from this PR to work around this bug for now.

@erlinghaugstad
Copy link

Me too, was trying to circumvent this by calling my own custom controller to fix this behaviour, but was unable to make it work.

Can this be released?

@excid3 excid3 merged commit 5f6a9b9 into excid3:main Sep 11, 2024
1 check 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.

4 participants