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

Morphing: Changes from JS libs that mutate the DOM are lost #1083

Open
weaverryan opened this issue Nov 28, 2023 · 16 comments
Open

Morphing: Changes from JS libs that mutate the DOM are lost #1083

weaverryan opened this issue Nov 28, 2023 · 16 comments

Comments

@weaverryan
Copy link

Hi!

Thank you for adding morphing! ❤️. I maintain the Symfony's LiveComponents package where we also morph. We frequently hit a problem with morphing + Stimulus that Turbo morphing also hits.

Reproducer: https://github.com/weaverryan/turbo-morph-tom-select-reproducer

tl;dr

  1. Initialize a package like TomSelect inside a Stimulus controller. I'm using TomSelect as an example, but it's not unique to this package:
export default class extends Controller {
     connect() {
        new TomSelect(this.element);
    }
}
<select data-controller="tom-select">...</select>
  1. Trigger a page refresh with morphing.

The result is that the rich TomSelect widget is lost. This is because TomSelect works by modifying the select (e.g. adding classes) and adding an entirely new <div> element after the select:

Screenshot 2023-11-28 at 9 22 44 AM

So, naturally, when the morph happens, the select element is reverted and the new <div> is lost entirely. However, because the <select data-controller="tom-select"> element was not removed and re-added to the page, disconnect() and connect() are not called again. Also, no Stimulus values were changed and no targets (if we added a target to the select) are added/removed.

The result is that we lose the TomSelect widget and there's no hook to reinitialize it.

Possible Solutions

Is there a solution for this? Or any thoughts? In Symfony LiveComponents, we have a complex MutationObserver system to track changes. But the correct solution would be something much simpler. Ideas:

A) Do we need to switch to a paradigm where we avoid JavaScript packages that mutate the DOM?
B) I know it #1019, there was temporarily a way for a Stimulus controller to "reconnect" when morphed. Is that needed?
C) Or, do we need some way to be able to mark an element to NOT be morphed (permanent)? This is actually not a great solution, as, in this case, you need to be aware of and find the new elements added by TomSelect and manually add some attribute (e.g. data-turbo-permanent). And if a Stimulus value did change and we did want to reinitialize TomSelect after a morph, we might need to manually destroy the <div> adde by TomSelect.

Note: TomSelect is an especially annoying/messy library. But from experience, this "JavaScript mutated the DOM and that was lost after morphing" will be a common issue.

Thanks and apologies if I've missed some thoughts on this already!

@brunoprietog
Copy link
Collaborator

What happens if you listen to the turbo:morph event to re-initialize it?

@weaverryan
Copy link
Author

weaverryan commented Nov 28, 2023

What happens if you listen to the turbo:morph event to re-initialize it?

Hmm, it does! The solution looks like:

<select
    data-controller="tom-select"
    data-action="turbo:morph@window->tom-select#reconnect"
>

Where reconnect is a custom method that kills tom-select and reinitializes it. A few things:

A) Unless I'm crazy (but I checked pretty closely), turbo:morph seems to be dispatched once the first time, then is dispatched exactly two times forever after.

B) It may be trick, but it would be more useful the turbo:morph (or a different event) were triggered on the element. I'm listening via window@. So I guess if we ever morph something else - like a turbo-frame - my controller will reinitialize unnecessarily? Or, better: could we add a new method in Stimulus - e.g. morphed() - that's called when something in the controller element was morphed?

Thanks!

@brunoprietog
Copy link
Collaborator

A) Unless I'm crazy (but I checked pretty closely), turbo:morph seems to be dispatched once the first time, then is dispatched exactly two times forever after.

This is because it is rendering the cached preview and then the response coming from the server. That's why the first time you only see one event and the subsequent times two.

B) It may be trick, but it would be more useful the turbo:morph (or a different event) were triggered on the element. I'm listening via window@. So I guess if we ever morph something else - like a turbo-frame - my controller will reinitialize unnecessarily? Or, better: could we add a new method in Stimulus - e.g. morphed() - that's called when something in the controller element was morphed?

From the Turbo side, dispatching an event from the element that has been morphed is entirely possible, I don't think it is the responsibility of Stimulus. I guess we would have to know the opinion of the maintainers. In the meantime, you could have an observer which do the job, something like:

// app/javascript/controllers/tom_select_controller.js

import { Controller } from "@hotwired/stimulus"

export default class extends Controller {
  initialize() {
    this.mutationObserver = new MutationObserver(this.handleMutation.bind(this))
  }

  connect() {
    this.mutationObserver.observe(this.element, { childList: true, subtree: true })
  }

  disconnect() {
    this.mutationObserver.disconnect()
  }

  handleMutation(mutations, observer) {
    for (const mutation of mutations) {
      if (mutation.type === 'childList' || mutation.type === 'attributes') {
        this.reconnect()
      }
    }
  }

  reconnect() {
    console.log('Element is changed. Reconnecting...')
    // Add your reconnect logic here
  }
}

seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Dec 3, 2023
Follow-up to [9944490][]
Related to [hotwired#1083]
Related to [@hotwired/turbo-railshotwired#533][]

The problem
---

Some client-side plugins are losing their state when elements are
morphed.

Without resorting to `MutationObserver` instances to determine when a
node is morphed, uses of those plugins don't have the ability to prevent
(without `[data-turbo-permanent]`) or respond to the morphing.

The proposal
---

This commit introduces a `turbo:before-morph` event that'll dispatch as
part of the Idiomorph `beforeNodeMorphed` callback. It'll give
interested parties access to the nodes before and after a morph. If that
event is cancelled via `event.preventDefault()`, it'll skip the morph as
if the element were marked with `[data-turbo-permanent]`.

Similarly, this commit re-purposes the new `turbo:morph` event to be
dispatched for every morphed node (via Idiomorph's `afterNodeMorphed`
callback). The original implementation dispatched the event for the
`<body>` element as part of `MorphRenderer`'s lifecycle. That event will
still be dispatched, since `<body>` is the first element the callback
will fire for. In addition to that event, each individual morphed node
will dispatch one.

This commit re-introduced test coverage for a Stimulus controller to
demonstrate how an interested party might respond. It isn't immediately
clear with that code should live, but once we iron out the details, it
could be part of a `@hotwired/turbo/stimulus` package, or a
`@hotwired/stimulus/turbo` package that users (or
`@hotwired/turbo-rails`) could opt-into.

[9944490]: hotwired@9944490
[hotwired#1083]: hotwired#1083
[@hotwired/turbo-railshotwired#533]: hotwired/turbo-rails#533
@brunoprietog
Copy link
Collaborator

Come to think of it, @seanpdoyle did a PR to Stimulus where he added callbacks when an element and targets changed. I think that might be something we should rescue, more so now with morphing. It is very common to use observers to execute actions and this could simplify it a lot. The example above is the best proof of it.

What do you think? I would love to see that PR merged.

hotwired/stimulus#460

seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Dec 7, 2023
Follow-up to [9944490][]
Related to [hotwired#1083]
Related to [@hotwired/turbo-railshotwired#533][]

The problem
---

Some client-side plugins are losing their state when elements are
morphed.

Without resorting to `MutationObserver` instances to determine when a
node is morphed, uses of those plugins don't have the ability to prevent
(without `[data-turbo-permanent]`) or respond to the morphing.

The proposal
---

This commit introduces a `turbo:before-morph` event that'll dispatch as
part of the Idiomorph `beforeNodeMorphed` callback. It'll give
interested parties access to the nodes before and after a morph. If that
event is cancelled via `event.preventDefault()`, it'll skip the morph as
if the element were marked with `[data-turbo-permanent]`.

Similarly, this commit re-purposes the new `turbo:morph` event to be
dispatched for every morphed node (via Idiomorph's `afterNodeMorphed`
callback). The original implementation dispatched the event for the
`<body>` element as part of `MorphRenderer`'s lifecycle. That event will
still be dispatched, since `<body>` is the first element the callback
will fire for. In addition to that event, each individual morphed node
will dispatch one.

This commit re-introduced test coverage for a Stimulus controller to
demonstrate how an interested party might respond. It isn't immediately
clear with that code should live, but once we iron out the details, it
could be part of a `@hotwired/turbo/stimulus` package, or a
`@hotwired/stimulus/turbo` package that users (or
`@hotwired/turbo-rails`) could opt-into.

[9944490]: hotwired@9944490
[hotwired#1083]: hotwired#1083
[@hotwired/turbo-railshotwired#533]: hotwired/turbo-rails#533
@dlegr250
Copy link

I ran into a similar issue where I was showing/hiding form fields based on a selected radio button value. I relied on the stimulus connect method to check which radio button was selected to determine whether to show/hide the additional content. When the form failed validation, turbo morph took over and wiped the DOM changes.

To resolve this, I did as the above posts recommended:

# visibility_controller.js
reconnect() {
  this.disconnect()
  this.connect()
}

connect() {
  # ...setup code...
  this.element.setAttribute("data-action", "turbo:morph@window->visibility#reconnect")
}

This works, and I could probably clean it up a bit by making a morphingReconnect module that I include in stimulus controllers that run into this issue.

But I want to make sure there's not a more conventional/official solution that the hotwire/turbo team is presenting? Is there a way to have per-page morphing? It would seem that forms that fail validation and re-render the form is a prime candidate for situations where the DOM has been manipulated client-side, and morphing from the server version will wipe those changes.

My stimulus controllers have been written the past few years with the assumption that the connect function is where I put my "setup" logic. Morphing seems to break that approach.

But I'm extremely stoked to see morphing being added! I was close to implementing my own solution a few months back until I saw the team was baking this in. Very excited, just want to make sure all my cool toys play nicely together :)

@jon-sully
Copy link

FYI @seanpdoyle's PR, #1097, should expose fine-grain controls that would give us better control and should fix this issue 👍

@donnysim
Copy link

I'm curious what about script tags? Usually when a body is replaced script tags are re-evaluated, but with morph this does not happen. If a javascript library modifies the DOM, like say, a script injects the debugbar html, the bar is removed on morph but never re-added because the existing script tag is morphed but not re-evaluated.

seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Jan 25, 2024
Follow-up to [9944490][]
Related to [hotwired#1083]
Related to [@hotwired/turbo-railshotwired#533][]

The problem
---

Some client-side plugins are losing their state when elements are
morphed.

Without resorting to `MutationObserver` instances to determine when a
node is morphed, uses of those plugins don't have the ability to prevent
(without `[data-turbo-permanent]`) or respond to the morphing.

The proposal
---

This commit introduces a `turbo:before-morph` event that'll dispatch as
part of the Idiomorph `beforeNodeMorphed` callback. It'll give
interested parties access to the nodes before and after a morph. If that
event is cancelled via `event.preventDefault()`, it'll skip the morph as
if the element were marked with `[data-turbo-permanent]`.

Along with `turbo:before-morph`, this commit also introduces a
`turbo:before-morph-attribute` to correspond to the
`beforeAttributeUpdated` callback that Idiomorph provides. When
listeners (like an `HTMLDetailsElement`, an `HTMLDialogElement`, or a
Stimulus controller) want to preserve the state of an attribute, they
can cancel the `turbo:before-morph-attribute` event that corresponds
with the attribute name (through `event.detail.attributeName`).

Similarly, this commit re-purposes the new `turbo:morph` event to be
dispatched for every morphed node (via Idiomorph's `afterNodeMorphed`
callback). The original implementation dispatched the event for the
`<body>` element as part of `MorphRenderer`'s lifecycle. That event will
still be dispatched, since `<body>` is the first element the callback
will fire for. In addition to that event, each individual morphed node
will dispatch one.

This commit re-introduced test coverage for a Stimulus controller to
demonstrate how an interested party might respond. It isn't immediately
clear with that code should live, but once we iron out the details, it
could be part of a `@hotwired/turbo/stimulus` package, or a
`@hotwired/stimulus/turbo` package that users (or
`@hotwired/turbo-rails`) could opt-into.

[9944490]: hotwired@9944490
[hotwired#1083]: hotwired#1083
[@hotwired/turbo-railshotwired#533]: hotwired/turbo-rails#533
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Jan 27, 2024
Follow-up to [9944490][]
Related to [hotwired#1083]
Related to [@hotwired/turbo-railshotwired#533][]

The problem
---

Some client-side plugins are losing their state when elements are
morphed.

Without resorting to `MutationObserver` instances to determine when a
node is morphed, uses of those plugins don't have the ability to prevent
(without `[data-turbo-permanent]`) or respond to the morphing.

The proposal
---

This commit introduces a `turbo:before-morph-element` event that'll
dispatch as part of the Idiomorph `beforeNodeMorphed` callback. It'll
give interested parties access to the nodes before and after a morph. If
that event is cancelled via `event.preventDefault()`, it'll skip the
morph as if the element were marked with `[data-turbo-permanent]`.

Along with `turbo:before-morph-element`, this commit also introduces a
`turbo:before-morph-attribute` to correspond to the
`beforeAttributeUpdated` callback that Idiomorph provides. When
listeners (like an `HTMLDetailsElement`, an `HTMLDialogElement`, or a
Stimulus controller) want to preserve the state of an attribute, they
can cancel the `turbo:before-morph-attribute` event that corresponds
with the attribute name (through `event.detail.attributeName`).

Similarly, this commit adds a new `turbo:morph-element` event to be
dispatched for every morphed node (via Idiomorph's `afterNodeMorphed`
callback). The original implementation dispatched the event for the
`<body>` element as part of `MorphRenderer`'s lifecycle. That event will
still be dispatched, since `<body>` is the first element the callback
will fire for. In addition to that event, each individual morphed node
will dispatch one.

This commit re-introduced test coverage for a Stimulus controller to
demonstrate how an interested party might respond. It isn't immediately
clear with that code should live, but once we iron out the details, it
could be part of a `@hotwired/turbo/stimulus` package, or a
`@hotwired/stimulus/turbo` package that users (or
`@hotwired/turbo-rails`) could opt-into.

[9944490]: hotwired@9944490
[hotwired#1083]: hotwired#1083
[@hotwired/turbo-railshotwired#533]: hotwired/turbo-rails#533
@danielfriis
Copy link

Could someone explain to me (a noob), why it wouldn't make sense for morphing to always disconnect and connect controllers?

@tpaulshippy
Copy link

Could be because disconnecting would mean losing state always. Isn't part of the point of morphing to maintain as much state as possible?

@donnysim
Copy link

donnysim commented Jun 4, 2024

The whole morphing thing really only works if you're implementing all the details, the moment a third party library, like a custom select, wysiwyg etc. is involved, it just messes things up like removing the created elements because it does not match what came from server and then the library explodes because those elements are not present and so on, so I'd say the real world use case is very slim.

@danielfriis
Copy link

Could be because disconnecting would mean losing state always. Isn't part of the point of morphing to maintain as much state as possible?

That's the problem. In my cases, most of the state is lost since I use various Stimulus controllers to add interactivity.

@chloerei
Copy link

chloerei commented Jun 4, 2024

@danielfriis Morph doesn't know how your controller(or other libs) manages state, and when attributes or child elements change, it doesn't need to be updated, partially updated, or completely updated.

Currently, you need to handle it yourself based on the morph event. data-turbo-permanent can skip morph. I wonder if there will be something like data-turbo-atomic in the future that allows elements to skip morph and replace completely.

@danielfriis
Copy link

@chloerei That makes sense. I did use data-turbo-permanent to begin with, but I realised it also preserves the element on page navigation, making it unusable when navigating between records.

I'm still a beginner in Rails and I'm just looking for the most "Rails-Way"-way of doing things. In this case, would that be to trigger connect on those Stimulus controllers (and other libs) that needs it after a morph has happened?

The only other way to handle this, would be to have the DOM changes included in new page renders from the server, correct?

@chloerei
Copy link

chloerei commented Jun 4, 2024

@danielfriis There aren't enough use cases to summarize best practices yet, I'm still learning.

data-turbo-permanent is good for elements that don't need morph, such as basecamp/local_time elements.

turbo:morph-element->controller#reconnect is good if you want the element to update and reconnect. hotwire_combobox does something similar https://github.com/josefarias/hotwire_combobox/pull/132/files .

Basecamp's card table and hey's calendar are good places to learn how 37signals uses morph.

Morph is still a bleeding edge feature, so if it's a barrier to learning Rails, you might want to turn it off first. It's an opt-in feature.

domchristie pushed a commit to domchristie/turbo that referenced this issue Jul 20, 2024
Follow-up to [9944490][]
Related to [hotwired#1083]
Related to [@hotwired/turbo-railshotwired#533][]

The problem
---

Some client-side plugins are losing their state when elements are
morphed.

Without resorting to `MutationObserver` instances to determine when a
node is morphed, uses of those plugins don't have the ability to prevent
(without `[data-turbo-permanent]`) or respond to the morphing.

The proposal
---

This commit introduces a `turbo:before-morph-element` event that'll
dispatch as part of the Idiomorph `beforeNodeMorphed` callback. It'll
give interested parties access to the nodes before and after a morph. If
that event is cancelled via `event.preventDefault()`, it'll skip the
morph as if the element were marked with `[data-turbo-permanent]`.

Along with `turbo:before-morph-element`, this commit also introduces a
`turbo:before-morph-attribute` to correspond to the
`beforeAttributeUpdated` callback that Idiomorph provides. When
listeners (like an `HTMLDetailsElement`, an `HTMLDialogElement`, or a
Stimulus controller) want to preserve the state of an attribute, they
can cancel the `turbo:before-morph-attribute` event that corresponds
with the attribute name (through `event.detail.attributeName`).

Similarly, this commit adds a new `turbo:morph-element` event to be
dispatched for every morphed node (via Idiomorph's `afterNodeMorphed`
callback). The original implementation dispatched the event for the
`<body>` element as part of `MorphRenderer`'s lifecycle. That event will
still be dispatched, since `<body>` is the first element the callback
will fire for. In addition to that event, each individual morphed node
will dispatch one.

This commit re-introduced test coverage for a Stimulus controller to
demonstrate how an interested party might respond. It isn't immediately
clear with that code should live, but once we iron out the details, it
could be part of a `@hotwired/turbo/stimulus` package, or a
`@hotwired/stimulus/turbo` package that users (or
`@hotwired/turbo-rails`) could opt-into.

[9944490]: hotwired@9944490
[hotwired#1083]: hotwired#1083
[@hotwired/turbo-railshotwired#533]: hotwired/turbo-rails#533
@Michoels
Copy link

Michoels commented Sep 8, 2024

I've been using some hackery with event listeners to automatically disconnect/reconnect the controller on morph.
My HTML markup doesn't change, and morphing is transparently managed by the controller.

I've found that this works well when wrapping JS libraries, such as Popper.js.

export default class extends Controller {
  connect() {
   window.addEventListener('turbo:morph', this.reconnect(this)); // Pass the controller to the callback. When `window` executes the callback, it will have a clean reference to the controller
  }

  disconnect() {
    window.removeEventListener('turbo:morph', this.reconnect(this));
  }
  
  reconnect = controller => event => {
    controller.disconnect();
    controller.connect();
   // Or re-initialize your library, etc.  
  }
}

@Michoels
Copy link

Update: Beware of recursion when calling connect!

Be careful when calling disconnect and connect.
I accidentally got connect to be recursively called on every morph, and wound up with an exponentially increasing population of event listeners and Popper objects. Browser was not happy, and performance slowed to a crawl after 10-15 refreshes :(

For idiot-proofing (mostly myself) I now recommend explicitly reinitializing without calling connect:

export default class extends Controller {
  connect() {
   window.addEventListener('turbo:morph', this.reconnect(this)); // Pass the controller to the callback. When `window` executes the callback, it will have a clean reference to the controller
  }

  disconnect() {
    window.removeEventListener('turbo:morph', this.reconnect(this));
  }
  
  reconnect = controller => event => {
    myLibrary.destroy();
    myLibrary.create();
   // Or re-initialize your library, etc.  
  }
}

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

9 participants