-
Notifications
You must be signed in to change notification settings - Fork 434
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
Extract and re-use element morphing logic #1234
Conversation
ab83929
to
49f5315
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanpdoyle, thanks for working on this. This will be a great refactor. I shared some comments for your consideration.
src/core/morphing.js
Outdated
import { FrameElement } from "../elements/frame_element" | ||
import { dispatch } from "../util" | ||
|
||
export function morphRefresh(currentElement, newElement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about moving the logic of morphRefresh
to PageMorphRenderer
and morphFrames
to FrameMorphRenderer
. We can leave this morphing.js
module in charge of lower-level primitives, but I think it's fine to move this higher-level behavior to those rendererer classes. For example, canRefreshFrame
, could be just a private method in PageMorphRenderer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to move those functions in this commit.
A secondary goal for this refactoring was in preparation for public consumption of morphing with something like event.detail.render = Turbo.morphElements(...)
or event.detail.render = Turbo.morphFrames(...)
in turbo:before-render
and turbo:before-frame-render
event listeners.
Exposing these methods publicly, then consuming them internally would provide consistent morphing (and event dispatching) throughout the codebase. Defining those methods on the renderer would force applications interested in experimenting with morphing to reach through the internal package structure to access the renderers.
Depending on our goals here, enabling access to internals in the absence of a public API might be the best way forward for now, until a design decisions for morphing (or Frame-specific refreshes, or morphing across URLs with differing pathnames, etc.) are settled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanpdoyle if we moved morphPage
and morphFrames
to the renderer objects, couldn't you then invoke MorphingFrameRenderer.renderElement
and MorphingPageRenderer.renderElement
if you just wanted to reuse the high level morphing+event dispatching logic from other places?
The problem I see with the current API morphFrames
, morphPage
, morphElements
, morphChildren
is that it mixes levels of abstraction. It lacks a bit of clarity. Leveraging the renderer classes to encapsulate the operations at the higher level of abstraction, and leaving the module for the low-level primitives make sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jorgemanrubia I've re-structured that module to only export morphElements
and morphChildren
.
In the interest of merging this re-structuring work, I've done my best to make sure these changes don't affect the public API.
If this work is merged, I'm planning on taking a few next steps:
- exporting
Turbo.morphPage
as an alias forMorphingPageRenderer.renderElement
- exporting
Turbo.morphFrame
as an alias forMorphingFrameRenderer.renderElement
- exporting
Turbo.morphElements
andTurbo.morphChildren
as-is - exploring ways to make
FrameElement.reload
integrate with morphing without needing to define aturbo:before-frame-render
event listener
As a follow-up to this PR, reviewing and merging #1028 and #1027 might help reduce the number of moving parts.
At the moment, the MorphingFrameRenderer
isn't utilized anywhere, and only serves as a class to hook a static renderElement
function off of. In order to achieve 4. from the list above, there might be an opportunity to re-arrange how Frame rendering is determined. I've opened #430 to explore what it'd take to introduce a FrameVisit
corollary to a Visit
, and plan on catching that branch up with these newer changes.
3f13dc9
to
456b93e
Compare
5e99865
to
346b551
Compare
Follow-up to [hotwired#1185][] Related to [hotwired#1192][] The `morph{Page,Frames,Elements}` functions --- Introduce a new `src/core/morphing` module to expose a centralized and re-usable `morphElements(currentElement, newElement)` function to be invoked across the various morphing contexts. Next, move the logic from the `MorphRenderer` into a module-private `IdomorphCallbacks` class. The `IdomorphCallbacks` class (like its `MorphRenderer` predecessor) wraps a call to `Idiomorph` based on its own set of callbacks. The bulk of the logic remains in the `IdomorphCallbacks` class, including checks for `[data-turbo-permanent]`. To serve as a seam for integration, the class retains a reference to a callback responsible for: * providing options for the `Idiomorph` * determining whether or not a node should be skipped while morphing The `MorphingPageRenderer` skips `<turbo-frame refresh="morph">` elements so that it can override their rendering to use morphing. Similarly, the `MorphingFrameRenderer` provides the `morphStyle: "innerHTML"` option to morph its children. Changes to the renderers --- To integrate with the new module, first rename the `MorphRenderer` to `MorphingPageRenderer` to set a new precedent that communicates the level of the document the morphing is scoped to. With that change in place, define the static `MorphingPageRenderer.renderElement` to mirror the other existing renderer static functions (like [PageRenderer.renderElement][], [ErrorRenderer.renderElement][], and [FrameRenderer.renderElement][]). This integrates with the changes proposed in [hotwired#1028][]. Next, modify the rest of the `MorphingPageRenderer` to integrate with its `PageRenderer` ancestor in a way that invokes the static `renderElement` function. This involves overriding the `preservingPermanentElements(callback)` method. In theory, morphing has implications on the concept of "permanence". In practice, morphing has the `[data-turbo-permanent]` attribute receive special treatment during morphing. Following the new precedent, introduce a new `MorphingFrameRenderer` class to define the `MorphingFrameRenderer.renderElement` function that invokes the `morphElements` function with `newElement.children` and `morphStyle: "innerHTML"`. Changes to the StreamActions --- The extraction of the `morphElements` function makes entirety of the `src/core/streams/actions/morph.js` module redundant. This commit removes that module and invokes `morphElements` directly within the `StreamActions.morph` function. Future possibilities --- In the future, additional changes could be made to expose the morphing capabilities as part of the `window.Turbo` interface. For example, applications could experiment with supporting [Page Refresh-style morphing for pages with different URL pathnames][hotwired#1177] by overriding the rendering mechanism in `turbo:before-render`: ```js addEventListener("turbo:before-render", (event) => { const someCriteriaForMorphing = ... if (someCriteriaForMorphing) { event.detail.render = Turbo.morphPage } }) addEventListener("turbo:before-frame-render", (event) => { const someCriteriaForMorphingAFrame = ... if (someCriteriaForMorphingAFrame) { event.detail.render = Turbo.morphFrames } }) ``` [hotwired#1185]: hotwired#1185 (comment) [hotwired#1192]: hotwired#1192 [PageRenderer.renderElement]: https://github.com/hotwired/turbo/blob/9fb05e3ed3ebb15fe7b13f52941f25df425e3d15/src/core/drive/page_renderer.js#L5-L11 [ErrorRenderer.renderElement]: https://github.com/hotwired/turbo/blob/9fb05e3ed3ebb15fe7b13f52941f25df425e3d15/src/core/drive/error_renderer.js#L5-L9 [FrameRenderer.renderElement]: https://github.com/hotwired/turbo/blob/9fb05e3ed3ebb15fe7b13f52941f25df425e3d15/src/core/frames/frame_renderer.js#L5-L16 [hotwired#1028]: hotwired#1028 [hotwired#1177]: hotwired#1177
346b551
to
922e488
Compare
morph() { | ||
morph(this) | ||
const morph = this.hasAttribute("children-only") ? | ||
morphChildren : | ||
morphElements | ||
|
||
this.targetElements.forEach((targetElement) => morph(targetElement, this.templateContent)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change adheres to the current style.
As a related change as a follow-up, #1240 proposes extending the existing [action="replace"]
and [action="update"]
with new [method="morph"]
attributes named in a way that corresponds to the value used in the <meta name="turbo-refresh-method" content="morph">
element.
@seanpdoyle thanks for your work here 🙏. I'm reviewing this one before the week ends. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work @seanpdoyle. Thank you so much 🙏.
I'll test this a bit with HEY this week and merge.
Follow-up to #1185
Related to #1192
The
morph{Page,Frames,Elements}
functionsIntroduce a new
src/core/morphing
module to expose a centralized andre-usable
morphElements(currentElement, newElement)
function to beinvoked across the various morphing contexts. Next, move the logic from
the
MorphRenderer
into a module-privateIdomorphCallbacks
class. TheIdomorphCallbacks
class (like itsMorphRenderer
predecessor) wraps acall to
Idiomorph
based on its own set of callbacks. The bulk of thelogic remains in the
IdomorphCallbacks
class, including checks for[data-turbo-permanent]
. To serve as a seam for integration, the classretains a reference to a callback responsible for:
Idiomorph
The
MorphingPageRenderer
skips<turbo-frame refresh="morph">
elementsso that it can override their rendering to use morphing. Similarly, the
MorphingFrameRenderer
provides themorphStyle: "innerHTML"
option tomorph its children.
Changes to the renderers
To integrate with the new module, first rename the
MorphRenderer
toMorphingPageRenderer
to set a new precedent that communicates thelevel of the document the morphing is scoped to. With that change in
place, define the static
MorphingPageRenderer.renderElement
to mirrorthe other existing renderer static functions (like
PageRenderer.renderElement, ErrorRenderer.renderElement, and
FrameRenderer.renderElement). This integrates with the changes
proposed in #1028.
Next, modify the rest of the
MorphingPageRenderer
to integrate withits
PageRenderer
ancestor in a way that invokes the staticrenderElement
function. This involves overriding thepreservingPermanentElements(callback)
method. In theory, morphing hasimplications on the concept of "permanence". In practice, morphing has
the
[data-turbo-permanent]
attribute receive special treatment duringmorphing.
Following the new precedent, introduce a new
MorphingFrameRenderer
class to define the
MorphingFrameRenderer.renderElement
function thatinvokes the
morphElements
function withnewElement.children
andmorphStyle: "innerHTML"
.Changes to the StreamActions
The extraction of the
morphElements
function makes entirety of thesrc/core/streams/actions/morph.js
module redundant. This commitremoves that module and invokes
morphElements
directly within theStreamActions.morph
function.Future possibilities
In the future, additional changes could be made to expose the morphing
capabilities as part of the
window.Turbo
interface.For example, applications could experiment with supporting Page
Refresh-style morphing for pages with different URL pathnames by
overriding the rendering mechanism in
turbo:before-render
: