Ensure response content is loaded before requestFinish is called.#1377
Open
armstrjare wants to merge 2 commits intohotwired:mainfrom
Open
Ensure response content is loaded before requestFinish is called.#1377armstrjare wants to merge 2 commits intohotwired:mainfrom
armstrjare wants to merge 2 commits intohotwired:mainfrom
Conversation
Previously the response content was loaded asynchronously within the FetchRequest delegate. Consequently, the requestFinished callback was called prior this finishing. The requestFinished callback triggers the toggling of the busy attribute on <html>/<turbo-frame>/<form>, the triggering of the afterSubmit callback which toggles the disabled state of form submitters, and the the turbo:submit-end event. For clients with high latency, this could result in janky behaviour when the busy/disabled state is removed a noticeable time before the response is loaded and rendered. This change removes this delay by ensuring the response is loaded and available prior to notifying the delegate and continuing execution of the Turbo behaviour.
Author
|
Fixed the |
Author
|
For anyone that wants this fixed for their app before this gets merged, here is a monkey-patch that gives effectively the same outcome: // Ensure that the responseHTML is loaded before the fetchRequest is declared finished
const originalReceive = Turbo.FetchRequest.prototype.receive
Turbo.FetchRequest.prototype.receive = async function(response) {
let fetchResponse = await originalReceive.call(this, response)
await fetchResponse.responseHTML
return fetchResponse
} |
Author
|
@jorgemanrubia is this something you guys would consider merging? let me know if there is any feedback on this |
|
We tested this in our project and, unfortunately, found that this change is causing some side effects. Specifically, when we use POST in a form, and our location doesn't change (we stay on the same page). Some events are not triggered, and some of our components simply break. I don't have much time to create a reproduction right now, but hopefully this helps. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously the response content was loaded asynchronously within the FetchRequest delegate. Consequently, the requestFinished callback was called prior this finishing. The requestFinished callback triggers the toggling of the busy attribute on
<html>/<turbo-frame>/<form>, the triggering of the afterSubmit callback which toggles the disabled state of form submitters, and the the turbo:submit-end event.For clients with high latency, this could result in janky behaviour when the busy/disabled state is removed a noticeable time before the response is loaded and rendered.
This change removes this delay by ensuring the response is loaded and available prior to notifying the delegate and continuing execution of the Turbo behaviour.
Resolves #766 #884 #1202
Replaces #1039
Note that I explored other solutions, such as having the FetchRequest
awaitthe success handlers, etc. on the delegate. Unfortunately, because of the behaviours tied to therequestFinishedcallback, this needs to be executed before the response is rendered by the other delegate callbacks, so that the various attribute updates on elements are processed before the PageSnapshot is taken for the cache, and before the existing DOM content is replaced as to not affect user callbacks.Ultimately, this solution practically resolves the issues, without impacting any other Turbo timings or behaviour.