Remove Circular Build Dependency #1049
Merged
Conversation
This file contains 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
|
Thanks! |
jorgemanrubia
pushed a commit
that referenced
this pull request
Nov 6, 2023
afcapel
pushed a commit
that referenced
this pull request
Nov 14, 2023
* Page refreshes This commit introduces the concept of page refresh. A page refresh happens when Turbo renders the current page again. We will offer two new options to control behavior when a page refresh happens: - The method used to update the page: with a new option to use morphing (Turbo currently replaces the body). - The scroll strategy: with a new option to keep it (Turbo currently resets scroll to the top-left). The combination of morphing and scroll-keeping results in smoother updates that keep the screen state. For example, this will keep both horizontal and vertical scroll, the focus, the text selection, CSS transition states, etc. We will also introduce a new turbo stream action that, when broadcasted, will request a page refresh. This will offer a simplified alternative to fine-grained broadcasted turbo-stream actions. Co-Authored-By: Jorge Manrubia <jorge@hey.com> * Introduce refresh attribute for frame element * Use dispatch util function in MorphRenderer * Don't morph frames flagged with "refresh=morph" as part of the full page refresh We will refresh those frames with morphing as part of the page refresh, so it does not make sense to morph them also as part of the full page refresh. If we do, we'll trigger a manual reload because the complete attribute will get removed. This aligns the upstreamed version with the private gem we've been using internally. This also fixes a couple of issues: - We don't want to manually reload all the remote turbo-frames, only those that are flagged with "refresh=morph". Regular remote frames will get reloaded automatically when removing their complete attribute during regular page refreshes. - Using idiomorph's "innerHTML" was resulting in a turbo-frame nested inside the target turbo-frame. I think its semantics is not morphing inner contents from both currentElement and newElement, but morphing newElement as the inner contents of currentElement. See #1019 (comment) * Delegate `StreamActions.refresh` to `Session` (#1026) The bulk of the `StreamActions.refresh` implementation was reaching through the global `window.Turbo` property, which itself was reaching through either global variables or the `Session`. This commit moves the implementation out of the `StreamActions` and into a new `Session.refresh(url, requestId)` method. With that change, all property access is encapsulated within the `Session`. To support that change, this commit also introduces the `StreamElement.requestId` property to read the `[request-id]` attribute. * Only morph the turbo-frame contents, not the frame itself I had removed this in #d1935bd15c85e0a8776afccb90393fd378aea2d2 but we do need innerHTML so that the outer frame don't get touched. The problem we had is that we were nesting turbo-frames, so using .children to only address the contents instead. * Don't patch fetch globally, import patched function where needed instead. We'll export a `fetch` function apps can import too. This can be necessary if an app, for example, is doing a manual `fetch` and it wants to prevent a reflected broadcast triggered by that request. We'll also make rails/request use the Turbo fetch version when available. That will be the preferred form. In general, we don't want apps to care about having to import or use Turbo's `fetch`, but it will be available for the cases an app needs it. We'll also expose the patched version via `Turbo.fetch`. See discussion #1019 (review) * Always morph remote turbo-frames when a page refresh happens Instead of flagging the frames you want to morph with an special attribute, we are always going to reload and refresh with morphing all the turbo-frames in the page. This simplifies the API as it removes the concern of categorizing remote turbo-frames on the programmer side when using page-refreshes. This is an idea by @afcapel, who raised the concern of the confusion the attribute replace=morph caused, and questioned its necessity. We can bring the old approach back if we find real cases that justify it. * Remove Circular Build Dependency (#1049) Resolves #1026 (comment) * Don't refresh automatically turbo-frames that descend from a [data-turbo-permanent] element Since we are now reloading all the remote frames in the page, we need to make sure we ignore those that are contained in elements to be preserved. * Don't add new [data-turbo-permanent] elements when they already exist in the page. It can happen that a same element exists but that idiomorph won't match it because some JS moved the element to another position. This will handle such scenarios automatically. * Don't reload stimulus controllers after morphing There was a flaw in the implementation: we wanted to reload the stimulus controllers when their element was effectively morphed because some attribute had changed. Our implementation was essentially reloading all the stimulus controllers instead. But, even if we implemented our original idea, we have changed our mind about it being the right call. The heuristic of "reload controllers when some attribute changed" came from some tests with legacy controllers that used dom attributes to track certain conditions. That doesn't seem like enough justification for the original idea. In general, you don't want to reload controllers unless their elements get disconnected or connected as part of the morphing operation. If it's important for a given controller to track changes to the dom, than it should do that (e.g: listening to connection of targets or outlets, or just with the mutation observer API), but we can't determine that from the outside. If we introduce some API here, it will be the opposite: an API to force a "reconnect" during morphing, but we need to see a real justification in practice. * Respect morphing and scroll preservation settings when handling form errors Turbo will render 422 responses to allow handling form errors. A common scenario in Rails is to render those setting the satus like: ``` render "edit", status: :unprocessable_entity ``` This change will consider such operations a "page refresh" and will also consider the scroll directive. * Morph remote turbo frames where the src attribute has changed There are some cases when we don't want to reload a remote turbo frame on a page refresh. This may be because Turbo has added a src attribute to the turbo frame element, but we don't want to reload the frame from that URL. Form example, when a form inside a turbo frame is submitted, Turbo adds a src attribute to the form element. In those cases we don't want to reload the Turbo frame from the src URL. The src attribute points to the form submission URL, which may not be loadable with a GET request. Same thing can happen when a link inside a turbo frame is clicked. Turbo adds a src attribute to the frame element, but we don't want to reload the frame from that URL. If the src attribute of a turbo frame changes, this signals that the server wants to render something different that what's currently on the DOM, and Turbo should respect that. This also matches the progressive enhancement behavior philosophy of Turbo. The behaviour results in the Turbo frame that the server sends, which is what would happen anyway if there was no morphing involved, or on a first page load. --------- Co-authored-by: Jorge Manrubia <jorge@hey.com> Co-authored-by: Sean Doyle <seanpdoyle@users.noreply.github.com> Co-authored-by: Jorge Manrubia <jorge.manrubia@gmail.com>
|
@seanpdoyle I think removing the See https://github.com/hotwired/turbo-rails/actions/runs/6971461623/job/18971576977 |
afcapel
added a commit
that referenced
this pull request
Nov 23, 2023
This was removed in #1049 but it's still used in the turbo-rails test suite. https://github.com/hotwired/turbo-rails/blob/c3ca7009c0759563ec65ecf1bc60a9af1ff33728/test/system/broadcasts_test.rb#L137
afcapel
pushed a commit
that referenced
this pull request
Nov 27, 2023
* Export StreamActions as property of window.Turbo This was removed in #1049 but it's still used in the turbo-rails test suite. https://github.com/hotwired/turbo-rails/blob/c3ca7009c0759563ec65ecf1bc60a9af1ff33728/test/system/broadcasts_test.rb#L137 * Remove circular dependency * Assert that Turbo interface is exposed via window global
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Dec 1, 2023
Follow-up to [hotwired#1049][] Follow-up to [hotwired#1073][] Related to [hotwired#1078][] Replace an internal `window.Turbo.session` access with an `import { session }` statement. Prior attempts at this change introduced circular dependencies. This attempt does not. Additionally, this change maintains the ability for consumers to access StreamActions through `window.Turbo.StreamActions`, while also enabling `import { StreamActions } from "@hotwired/turbo"` (or `from "@hotwired/turbo-rails"`). [hotwired#1049]: hotwired#1049 [hotwired#1073]: hotwired#1073 [hotwired#1078]: hotwired#1078
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolves #1026 (comment)