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

Navigating to page with non-successful response seems to reload javascript #1190

Closed
RemcodM opened this issue Feb 19, 2024 · 9 comments
Closed

Comments

@RemcodM
Copy link

RemcodM commented Feb 19, 2024

We are using Turbo Drive 7.3.0, but is also happens on our 8.0.2 branch that we are working on.

In some cases, when navigating with Turbo Drive, we can navigate to a page returning a non-successful error response, such as a error 403 because the user has no permission to that page.

When this happens, Turbo does not seem to keep track of the script tags in the header correctly. They seem to be evaluated on top of the existing JavaScript, causing all kinds of issues, such as: Uncaught SyntaxError: redeclaration of const ...

We are using data-turbo-track="reload" on our scripts, however, the assets have not been changed between the requests. So we do not expect a reload at all.

The problem does not seem to occur once we return the forbidden page with a successful response code (200). Once we change it back to 403, or even 404, 400 or 500, the problem returns.

@afcapel
Copy link
Collaborator

afcapel commented Feb 19, 2024

@RemcodM can you provide a minimal rails app showcasing the problem?

@Ambient-Impact
Copy link

I've also been seeing the same re-evaluation of scripts on our Drupal site: you can trigger the re-evaluation by going to any article (e.g. the Omnipedia article) and clicking the Edit tab which gives you a 403 error; after navigating to one or two other 200 response links, you'll start seeing the console debug messages duplicate. I can try and put together a reduced test case when I have some time.

@Ambient-Impact
Copy link

I just poked around in the Firefox dev tools a bit to try and see what's going on, and what I noticed is that the <head> element in the 200 response pages is no longer in the DOM after a visit to the 4xx page, where a different <head> element is present - I have no idea why this would be happening, but it would certainly explain the re-evaluation of scripts if it is indeed a new <head> element being added to the DOM with the <script> elements from the previous visit already present.

@afcapel
Copy link
Collaborator

afcapel commented Feb 21, 2024

Yes, Turbo does this intentionally when you mark a script tag with data-turbo-track="reload". It's the only way to ensure the client runs the latest JS after a deploy.

@afcapel
Copy link
Collaborator

afcapel commented Feb 21, 2024

I'm going to close this, since it seems expected behavior. But feel free to reopen with a test case if you find a bug.

@afcapel afcapel closed this as completed Feb 21, 2024
@Ambient-Impact
Copy link

@afcapel We don't mark the scripts with the attribute at all if you view source, and what occurs is still a Turbo visit, not a full page load. If this was intended behaviour, shouldn't it force a full page load instead of doing a Turbo visit?

@afcapel
Copy link
Collaborator

afcapel commented Feb 21, 2024

Seems that you have a meta tag with data-turbo-track="reload"?

Screenshot 2024-02-21 at 15 50 58

I'd try removing it and see if that helps. But, also, as I said, happy to reopen the issue if you can provide a minimal reproduction.

@Ambient-Impact
Copy link

@afcapel I just tried removing it but it had no effect. That meta tag only ever changes when navigating between the site front-end and admin sections, where the Drupal theme changes and it does force a full page load as it's supposed to, so I would have been surprised if it had an effect. I appreciate you getting back to me, and I'll see if I can throw together a minimal reproduction.

@Ambient-Impact
Copy link

Ambient-Impact commented Feb 21, 2024

So I did some debugging in dev tools and discovered that the ErrorRenderer class always replaces the <head> outright:

documentElement.replaceChild(this.newHead, head)

This probably makes sense for some apps, but in our cases it seems like it's creating a weird in between state where a full page load has not occurred so scripts executed previously still linger in the JavaScript environment but the scripts get re-evaluated by ErrorRenderer:

this.activateScriptElements()

In our Drupal app, the 4xx pages contain the same linked JavaScript files as most other pages, so this means Turbo re-evaluates all the same JavaScript files on an error response.

The full stack trace:

replaceHeadAndBody (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#3719)
render (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#3713)
renderSnapshot (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#1535)
render (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#1495)
renderError (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#4985)
loadResponse (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#2506)
render (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#2710)
loadResponse (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#2495)
visitRequestFailedWithStatusCode (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#2780)
recordResponse (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#2482)
requestFailedWithResponse (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#2616)
receive (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#856)
perform (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#831)
issueRequest (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#2458)
visitStarted (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#2751)
start (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#2408)
startVisit (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#3290)
visitProposedToLocation (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#2742)
visitProposedToLocation (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#5337)
proposeVisit (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#3280)
visit (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#5201)
followedLinkToLocation (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#5326)
clickBubbled (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#1635)
clickCaptured (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#1624)
start (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#1610)
start (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#5155)
start (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#5602)
<anonymous> (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#6573)
<anonymous> (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#8)
<anonymous> (https://omnipedia.ddev.site/modules/contrib/refreshless/modules/refreshless_turbo/vendor/%40hotwired/turbo/dist/turbo.es2017-umd.js?v=8.0.2#9)

Using that clue, I took a look at Visit.loadResponse():

await this.view.renderError(PageSnapshot.fromHTMLString(responseHTML), this)

and replaced that line with:

await this.renderPageSnapshot(PageSnapshot.fromHTMLString(responseHTML), false);

reloaded the page, and bingo, it no longer re-evaluates all of our JavaScript no matter how many 4xx error pages I visit. It's probably not the best way to do this and may break stuff I'm not aware of.


Given that, is there a way to configure Turbo to use the normal renderer rather than the error renderer?

Ambient-Impact added a commit to neurocracy/omnipedia that referenced this issue Feb 21, 2024
hotwired/turbo#1190

Also combined patches into one file to keep things simple.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants