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

trubo:load gets triggered twice when GET request is a redirect 302 #428

Closed
joker-777 opened this issue Oct 21, 2021 · 9 comments · Fixed by #563
Closed

trubo:load gets triggered twice when GET request is a redirect 302 #428

joker-777 opened this issue Oct 21, 2021 · 9 comments · Fixed by #563

Comments

@joker-777
Copy link

joker-777 commented Oct 21, 2021

Hi, is this on purpose and if so, how can I make sure that the first call doesn't do anything?

I created a test where you can test this problem

Start there the server bundle exec rails s and open the page localhost:3000. You
will see a link that points to page 1 but then gets redirected to page 2. On the
first page load, you will see an alert which you then will see twice after
clicking on this link.

@joker-777 joker-777 changed the title trubo:load gets treggered twice when GET request is a redirect 302 trubo:load gets triggered twice when GET request is a redirect 302 Oct 25, 2021
@tleish
Copy link
Contributor

tleish commented Nov 23, 2021

@joker-777 - this is caused because turbo loads 2 urls, the initial URL and the redirected URL. Can you describe how this is causing an issue for you?

@joker-777
Copy link
Author

@tleish Thanks a million for your response. We use trubo:load to execute the javascript for a certain page. Having it triggered for a page that in the end doesn't get rendered but instead redirected to another page isn't something I would expect. It also can create errors in the javascript code for this specific page. What do you recommend here?

@tleish
Copy link
Contributor

tleish commented Nov 24, 2021

@joker-777 - I understand it might not be expected... although for redirects, turbo actually does fetch and load the initial url and the redirected URL.

Do you have an actual use case where turbo:load on redirect is causing an issue?

@joker-777
Copy link
Author

@tleish In our case, the turbo:load callback executes certain modules based on the current URL. This module will ofter modify the dom and also trigger analytics events like a page view event. Modifying the dom raises an error because the dom doesn't contain the expected elements and also the page view event is incorrect because the user just got redirected, he didn't see the page. I'm happy to create an example app if it is still needed.

@gingerlime
Copy link

I think also semantically, it feels strange to have turbo:load to fire on redirect... the only page that really "loads" is the destination page, not the redirected-from page? (perhaps in the same way that you don't load JS and CSS on a 302, your browser redirects and only loads JS, CSS etc on a 200)

@joker-777
Copy link
Author

By the way, the same applies also when the page gets reloaded because of the data-turbo-track="reload" attribute. I would expect that turbo:load gets only called once but it actually gets called twice.

@feliperaul
Copy link
Contributor

I just hit this and I can't think of a way to escape as it is.

Our page also contains javascript that as soon turbo:load fires, it will dispatch JS requests for analytics, counting 2 views while only one actually happened.

But that's not all. We have pages that once turbo:load fires, dispatch JS POST requests that update the model's lock_version (Rails's OptimisticLocking).

Since turbo:load is firing twice, the app totally breaks because when the response's HTML actually renders on the screen, the "ghost" render that was previously invoked by Turbo fired the JS POST requests updating the lock_version unadvertly, so the user already has a stale version in his screen.

The browser never triggers 'DOMContentLoaded' twice after receiving a 302 or 303 response, but Turbo is.

This has nothing to do with idempotency; the code is totally idempotent as it is.

Lastly, there's the performance penalty: the browser is initializing a huge Vue application twice, making JS requests to fetch data, and so on. If it was only this, and they were only GET requests, it would be a minor bug, but considering the non-idempotent requests JS can make once turbo:load gets fired, this is actually a major problem on pages that can be loaded after a redirect.

@gingerlime
Copy link

Thank you so much for fixing it @feliperaul 🏆

@joker-777
Copy link
Author

@feliperaul Amazing!

seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Mar 2, 2023
After the changes made in [@hotwired/turbohotwired#867][] and changes made in
[@hotwired/turbo-railshotwired#428][] (the canonical server-side
implementation), Turbo expects full HTML documents in response to
requests with `Turbo-Frame:` headers.

Prior to this commit, the `FrameController` compensated for missing
pieces of an HTML document by taking an HTML "snapshot" of the current
page through the `<html>` element's [outerHTML][].

This commit changes the `fetchResponseLoaded` callback to read the
`responseHTML` directly from the `FetchResponse`, since that will be a
fully formed HTML document in Turbo v7.3.0 and later.

To support that change, this commit also updates various
`src/test/fixtures` files to render fully-formed HTML documents.

[@hotwired/turbohotwired#867]: hotwired#867
[@hotwired/turbo-railshotwired#428]: hotwired/turbo-rails#428
[outerHTML]: https://developer.mozilla.org/en-US/docs/Web/API/Element/outerHTML
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Mar 2, 2023
After the changes made in [@hotwired/turbohotwired#867][] and changes made in
[@hotwired/turbo-railshotwired#428][] (the canonical server-side
implementation), Turbo expects full HTML documents in response to
requests with `Turbo-Frame:` headers.

Prior to this commit, the `FrameController` compensated for missing
pieces of an HTML document by taking an HTML "snapshot" of the current
page through the `<html>` element's [outerHTML][].

This commit changes the `fetchResponseLoaded` callback to read the
`responseHTML` directly from the `FetchResponse`, since that will be a
fully formed HTML document in Turbo v7.3.0 and later.

To support that change, this commit also updates various
`src/test/fixtures` files to render fully-formed HTML documents.

[@hotwired/turbohotwired#867]: hotwired#867
[@hotwired/turbo-railshotwired#428]: hotwired/turbo-rails#428
[outerHTML]: https://developer.mozilla.org/en-US/docs/Web/API/Element/outerHTML
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Mar 2, 2023
After the changes made in [@hotwired/turbohotwired#867][] and changes made in
[@hotwired/turbo-railshotwired#428][] (the canonical server-side
implementation), Turbo expects full HTML documents in response to
requests with `Turbo-Frame:` headers.

Prior to this commit, the `FrameController` compensated for missing
pieces of an HTML document by taking an HTML "snapshot" of the current
page through the `<html>` element's [outerHTML][].

This commit changes the `fetchResponseLoaded` callback to read the
`responseHTML` directly from the `FetchResponse`, since that will be a
fully formed HTML document in Turbo v7.3.0 and later.

To support that change, this commit also updates various
`src/test/fixtures` files to render fully-formed HTML documents.

[@hotwired/turbohotwired#867]: hotwired#867
[@hotwired/turbo-railshotwired#428]: hotwired/turbo-rails#428
[outerHTML]: https://developer.mozilla.org/en-US/docs/Web/API/Element/outerHTML
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Jul 21, 2023
After the changes made in [@hotwired/turbohotwired#867][] and changes made in
[@hotwired/turbo-railshotwired#428][] (the canonical server-side
implementation), Turbo expects full HTML documents in response to
requests with `Turbo-Frame:` headers.

Prior to this commit, the `FrameController` compensated for missing
pieces of an HTML document by taking an HTML "snapshot" of the current
page through the `<html>` element's [outerHTML][].

This commit changes the `fetchResponseLoaded` callback to read the
`responseHTML` directly from the `FetchResponse`, since that will be a
fully formed HTML document in Turbo v7.3.0 and later.

To support that change, this commit also updates various
`src/test/fixtures` files to render fully-formed HTML documents.

[@hotwired/turbohotwired#867]: hotwired#867
[@hotwired/turbo-railshotwired#428]: hotwired/turbo-rails#428
[outerHTML]: https://developer.mozilla.org/en-US/docs/Web/API/Element/outerHTML
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Sep 10, 2023
After the changes made in [@hotwired/turbohotwired#867][] and changes made in
[@hotwired/turbo-railshotwired#428][] (the canonical server-side
implementation), Turbo expects full HTML documents in response to
requests with `Turbo-Frame:` headers.

Prior to this commit, the `FrameController` compensated for missing
pieces of an HTML document by taking an HTML "snapshot" of the current
page through the `<html>` element's [outerHTML][].

This commit changes the `fetchResponseLoaded` callback to read the
`responseHTML` directly from the `FetchResponse`, since that will be a
fully formed HTML document in Turbo v7.3.0 and later.

To support that change, this commit also updates various
`src/test/fixtures` files to render fully-formed HTML documents.

[@hotwired/turbohotwired#867]: hotwired#867
[@hotwired/turbo-railshotwired#428]: hotwired/turbo-rails#428
[outerHTML]: https://developer.mozilla.org/en-US/docs/Web/API/Element/outerHTML
shiftyp pushed a commit to shiftyp/ts-turbo that referenced this issue Sep 11, 2023
After the changes made in [@hotwired/turbohotwired#867][] and changes made in
[@hotwired/turbo-railshotwired#428][] (the canonical server-side
implementation), Turbo expects full HTML documents in response to
requests with `Turbo-Frame:` headers.

Prior to this commit, the `FrameController` compensated for missing
pieces of an HTML document by taking an HTML "snapshot" of the current
page through the `<html>` element's [outerHTML][].

This commit changes the `fetchResponseLoaded` callback to read the
`responseHTML` directly from the `FetchResponse`, since that will be a
fully formed HTML document in Turbo v7.3.0 and later.

To support that change, this commit also updates various
`src/test/fixtures` files to render fully-formed HTML documents.

[@hotwired/turbohotwired#867]: hotwired#867
[@hotwired/turbo-railshotwired#428]: hotwired/turbo-rails#428
[outerHTML]: https://developer.mozilla.org/en-US/docs/Web/API/Element/outerHTML
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants