-
Notifications
You must be signed in to change notification settings - Fork 419
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
Fix turbo:load
firing twice after a redirect and avoid double render
#563
Conversation
Looks like we have some test failures? |
* main: Fix lint violations
Want to take a look at this @feliperaul? |
@dhh Hi David, thanks for bumping me on this. I wish I could help further... the fact is that when I issued the PR I added a test for my change, and the test is passing on Firefox, but after literally hours of attempts I couldn't make it pass on Chrome (it always fails with this Sorry for issuing a PR with a failing test, but I mentioned this on the comment above hoping someone could chip in and get this test passing on Chrome so this could be merged. I hit a wall here. Maybe you could cc someone that could give it a try? |
I wonder if visits should handle redirects internally, or have some way of determining a visit that originated from a redirect e.g. via a property on the |
* main: Allow frames to scroll smoothly into view (hotwired#607) Export Type declarations for `turbo:` events (hotwired#452) Add .php as a valid isHTML extension (hotwired#629) Add original click event to 'turbo:click' details (hotwired#611) Drive Browser tests with `playwright` (hotwired#609) Allow Turbo Streams w/ GET via `data-turbo-stream` (hotwired#612) Only update history when Turbo visit is renderable (hotwired#601) Support development ChromeDriver version overrides (hotwired#606) Turbo stream source (hotwired#415) Expose Frame load state via `[complete]` attribute (hotwired#487) fix(ie/edge): form.method='delete', raises Invalid argument. (hotwired#586) Do not declare global types/constants (hotwired#524) Defensively create custom turbo elements (hotwired#483) Use `replaceChildren` in StreamActions.update (hotwired#534)
Hi there! This is my first contribution to turbo.
This fixes #428, #526 and may fix #537 as well.
Turbo is firing
turbo:load
twice when the server responds with a redirect (regardless of status code, it happens with 302, 303).This is harmful because per the official docs,
turbo:load
replaces the nativeDOMContentLoaded
, which never fires twice on a redirect. The docs clearly stateturbo:load
should fire only once.Our team had to halt the migration to Turbo because of this; not only analytics events listening to
turbo:load
were registering two visits while there actually just one, but many pages hadGET
and evenPOST
javascript events fired onturbo:load
, some of them that even updated a Rails model'slock_version
on the backend, making it so that when the second render was actually mounted on the screen it couldn't save the model because it was already stale (due to the previous "ghost" turbo:load inadvertently firing).Our code is idempotent; the fact is Turbo is firing
turbo:load
twice and it's impossible to distinguish one from another (let alone this shouldn't be a extra burden on the developer).I'm aware of the pull request at #516 , but it appears to only solve the double render, but the event will still fire twice;
I tested it and it does not solve the double
turbo:load
firing. This pull request solves both issues.PS: I added a test that is passing on Firefox, but after hours of attempts I couldn't make it pass on Chrome (it fails with a
StaleElementReference
, and I couldn't find any test on the suite that asserted events after a redirect).You will also see
NavigationTests - skip link with hash-only path moves focus and changes tab order
failing, but it is already failing onmain
so it didn't break due to this pull request.