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

Flowbite's handling of Turbo is using the wrong strategy #796

Open
daniel-rikowski opened this issue Feb 5, 2024 · 9 comments
Open

Flowbite's handling of Turbo is using the wrong strategy #796

daniel-rikowski opened this issue Feb 5, 2024 · 9 comments

Comments

@daniel-rikowski
Copy link

daniel-rikowski commented Feb 5, 2024

tl;dr: The current approach for integrating Turbo with Flowbite is using a mismatched strategy. As far as I can tell, it will only ever work for a certain subset of Turbo-enabled web applications. Events don't work for every case and should be replaced with MutationObserver.


Details

Sorry for the long wall of text, but I believe this is necessary to understand the problem.

I spend some hours over the weekend to make Flowbite work with Turbo, but had no actual success. As soon as I used more than just the page cache feature, simple visits or most-of-the-page Turbo frames, I encountered dead ends.

The unpublished fix in #760 did help at first, but it then revealed a huge conceptual problem, which I think cannot be tackled with the current approach.

Let me explain 😃 Most previous issues regarding Turbo were about missing initialization on frame navigations or after stream actions, or about broken re-initialization: #450, #760, #698, #697, #454

They are fixed now, but they all are centered around a certain aspect:

Some component instances need to be initialized or reinitialized after a Turbo operation.

For a simple application, where Turbo is used mostly as a page cache, this is all you need.

But as soon as you start using Turbo in a more fine-grained approach, with many frames and stream actions, you will also encounter the opposite problem:

Some component instances must not be reinitialized after a Turbo operation.

There are no reported issues regarding this topic yet, besides the odd "no, this fix still doesn't work for me" comments.
(Most likely because fine-grained decomposed Turbo pages utilize stream actions, which didn't work until #760.)

Example

You have a modal, make it visible, and then update a completely unrelated portion of the DOM, either via frame navigation or a stream action. Then the subsequent initFlowbite/initModals will re-initialize all modals, even the one currently visible.

This leads to an inconsistent state of the page, because the new instance of Modal thinks, it is hidden, but the
DOM nodes of the modal still have the TailwindCSS classes, which make it visible. The backdrop is removed, too, so you are
left with kind of a "zombie" modal, not fully open, not fully closed.

Depending on the app (or the component) that might not be a problem, e.g. when no Modal is visible during a Turbo operation. Or when it is actually the modal, which is replaced during that operation, because then you get "fresh" HTML with the correct classes.

In apps where this actually is a problem, the idea to make initFlowbite/initModals idempotent (as proposed in #119 (comment)) only would help in those cases, where components are not replaced. In other words, in such cases where the DOM nodes of a component get replaced by a Turbo operation with HTML of the same component, then that component must actually be reinitialized, and an idempotent init function would wrongly skip it.

Why just Flowbite?

The reason why other frameworks don't have these problems is that their implementation of a component instance manager stores the instances together with the DOM node element, in Foundation indirectly using jQuery's data, for example. (See https://github.com/foundation/foundation-sites/blob/develop/js/foundation.core.plugin.js, https://github.com/jquery/jquery/blob/main/src/data/Data.js)

So on those frameworks, if any operation of Turbo replaces a part of the DOM, any affected component instances normally are gone as well, as opposed to Flowbite, where the DOM nodes and their respective component instances are loosely coupled only through an ID string. I like Flowbite's approach better, but it requires a more fine-grained approach, as I explain below.

The wrong strategy

The core of the problem lies in the event-based approach coupled with a full (re-)initialization after turbo:load, turbo:frame-load and now the custom turbo:after-stream-render event.

Turbo's fine-grained modifications of the DOM and the current catch-all approach of (re-)initializing everything on the page conceptually don't match.

A better strategy

The proper solution is to only (re-)initialize components in those parts of the DOM which actually have been changed.

I wrote a simple proof-of-concept handler for Turbo which utilizes MutationObserver and - as far as I can tell - only requires superficial modifications to the current Flowbite JS code:

const observer = new MutationObserver((mutationList, observer) => {
    const modals = window.FlowbiteInstances.getInstances('Modal') // Hack, because `instances` is not exported
    for (const mutation of mutationList) {
        // Destroy components from removed DOM nodes
        for (const root of mutation.removedNodes) {
            const ids = collectIDs(root) // Implemented elsewhere
            const removed = ids.map(id => modals[id]).filter(Boolean)
            removed.forEach(modal => modal.destroyAndRemoveInstance())
        }

        // Initialize components from added DOM nodes
        for (const root of mutation.addedNodes) {
            if (root.nodeType !== Node.ELEMENT_NODE) {
                continue // Most often a #text node
            }
            initModals(root) // <== New parameter required in Flowbite initializers
        }
    }
})

observer.observe(document.body, { childList: true, subtree: true })

As you can see, the overall principle is not that complicated, but it requires some modifications to the init* functions, most notably adding an optional parameter to specify which subtree to initialize.

The required changes look like this:

export function initModals(root = document) { // <== New optional parameter
    // initiate modal based on data-modal-target
    const targets = Array.from(root.querySelectorAll('[data-modal-target]'))
    // This part is ugly: `querySelectorAll` only considers child nodes and since a non-document node could already
    // be the component target, we must explicitly check the root node, too.
    if (root.matches('[data-modal-target]')) {
        targets.push(root)
    }
    targets.forEach(function ($triggerEl) {
        var modalId = $triggerEl.getAttribute('data-modal-target');
        var $modalEl = document.getElementById(modalId); //
        // ...
    }
    // ...
}

I tested this extensively with modals, and it seems to work much better:

  • New nodes/modals are initialized.
  • Replaced nodes/modals are destroyed then initialized again.
  • Removed nodes/modals are destroyed.
  • Unchanged nodes/modals remain untouched.

Most likely, my code can be optimized and perhaps there are some edge cases I didn't consider, especially in the other components I didn't test.

The beauty of this solution is that it is almost completely agnostic to Turbo and should work with every other library which modifies the DOM in a similar fashion.

The only Turbo-specific thing is the event name for the initial page load:

document.addEventListener('turbo:load', () => {
    initFlowbite()
    observeChanges()
})

On the other hand, I'm not sure how my solution behaves in conjunction with JS frameworks like Vue or React.
I can imagine there might be conflicts, but this might also make some Vue wrappers obsolete, especially if their only purpose is fine-grained initialization.

But I do have some experience with Stimulus:
Before coming to this solution, I used a Stimulus controller for manual initialization (i.e. explicitly calling new Modal) to tackle the problem described above. This worked well, but the observer approach made it obsolete, which is not surprising, since Stimulus also uses MutationObserver. (It now only provides an "open modal after fame load" feature that I wanted, since that specific feature was removed from Flowbite #141 (comment) )

As much as #760 was pivotal in finding a proper solution, I must admit that the Turbo developers were right in not adding that event. I once asked for a similar feature in order to apply the same "sledgehammer" approach (hotwired/turbo#425 (comment) 😌)

Final words

Thank you for reading this. I'm hoping my proposal can be integrated into Flowbite. The changes should be small, albeit at numerous locations, i.e. all init functions. They should not affect non-Turbo users at all, too, but they would provide proper support for all Turbo-related use cases.

@Doddlin
Copy link

Doddlin commented Feb 5, 2024

Could this (resolve?) relate to #793 ?

@daniel-rikowski
Copy link
Author

I don't think so. More at #793 (comment)

@zoltanszogyenyi
Copy link
Member

Hey @daniel-rikowski,

Thanks a lot for the very informative issue and explanations!

I've recently merged a PR that should address this issue:

3f2ed0a

Can you please have a look at it whether it solves the turbo reload issue?

Otherwise, if you create a PR based on this I will review it and test it and merge it in case it will permanently resolve these turbo reload issues with Flowbite :) you'll get a spot as one of the contributors of Flowbite on the homepage too!

Cheers,
Zoltan

@opedrosouza
Copy link

I don't know if my issue is related to this, but based on the context I think so.

Basically, I have a turbo_frame(id: :modal) on the body of my HTML, and all my CRUD operations are handled inside the modals.

When I open a modal on the route app/students then navigate to the route app/parents and go back to the app/students route the modal that I have opened before blinks on the screen.

Screen.Recording.2024-02-06.at.22.23.03.mov

here is a screen record of the behavior I'm facing.

@daniel-rikowski
Copy link
Author

daniel-rikowski commented Feb 7, 2024

Hello @zoltanszogyenyi, thank you for attending this!

Yes, I'm aware of #760, it filled the last gap on the "must reinitialize" side of Turbo, namely after changes caused by stream actions. (Also it enabled me to investigate the problem in the first place.)

But it does not solve the "must not reinitialize" side of Turbo, for parts of the page which must not be touched. Even though the approach is very elegant, it funnels even the most minor change of the page through undifferentiated, broad-brushed "reinitialize all the things" init-* handlers. (Imagine relevant meme here 😆)

Still, at the moment #760 only makes things better. ( :shipit: ?)

I'm tempted to try a PR, although I find the step to delve into a new and foreign codebase to be daunting. Perhaps I'll try to create a small monkey patch first, to quickly aid people in determining if their specific Turbo-related problem is actually this problem.

@daniel-rikowski
Copy link
Author

daniel-rikowski commented Feb 7, 2024

Hello @opedrosouza

it might be this problem, but considering the specific timing of the blinking in the screen recording, my first guess would be a "stale cache" or a preview problem. (i.e. the Turbo cache, not the browser cache)

You can test this quickly, by adding this to all pages, i.e. the layout file:

<head>
  ...
  <meta name="turbo-cache-control" content="no-cache">
</head>

If the problem is gone (and your site feels considerably slower 😢) then you can try to gradually reintroduce Turbo caching,
first by using no-preview instead of no-cache. If that works, then by only applying it to certain pages or by manually clearing the cache. (Both possible from JS) Also [data-turbo-temporary] may help in your specific case. For details see https://turbo.hotwired.dev/handbook/building#opting-out-of-caching

Perhaps in the future Flowbite can react to turbo:before-cache or [data-turbo-preview], perhaps even just by providing some documentation for typical use cases. (Modals via turbo-frame seems to be one of those.)

@zoltanszogyenyi
Copy link
Member

Hello @ zoltanszogyenyi, thank you for attending this!

Yes, I'm aware of #760, it filled the last gap on the "must reinitialize" side of Turbo, namely after changes caused by stream actions. (Also it enabled me to investigate the problem in the first place.)

But it does not solve the "must not reinitialize" side of Turbo, for parts of the page which must not be touched. Even though the approach is very elegant, it funnels even the most minor change of the page through undifferentiated, broad-brushed "reinitialize all the things" init-* handlers. (Imagine relevant meme here 😆)

Still, at the moment #760 only makes things better. ( :shipit: ?)

I'm tempted to try a PR, although I find the step to delve into a new and foreign codebase to be daunting. Perhaps I'll try to create a small monkey patch first, to quickly aid people in determining if their specific Turbo-related problem is actually this problem.

Thanks for getting back to me! I'll push the previous in the next release this week, I'm very open to collaborating and finding a permanent solution for the Turbo components - our codebase is relatively simple, in the sense that we have component classes for stuff like Accordions, Modals and there we have all methods and local variables.

One tricker part is the FlowbiteInstance manager, but you can find more info about it here: https://flowbite.com/docs/getting-started/javascript/

I'm also available to chit-chat on Discord!

Cheers,
Zoltan

@opedrosouza
Copy link

Hello @opedrosouza

it might be this problem, but considering the specific timing of the blinking in the screen recording, my first guess would be a "stale cache" or a preview problem. (i.e. the Turbo cache, not the browser cache)

You can test this quickly, by adding this to all pages, i.e. the layout file:

<head>
  ...
  <meta name="turbo-cache-control" content="no-cache">
</head>

If the problem is gone (and your site feels considerably slower 😢) then you can try to gradually reintroduce Turbo caching, first by using no-preview instead of no-cache. If that works, then by only applying it to certain pages or by manually clearing the cache. (Both possible from JS) Also [data-turbo-temporary] may help in your specific case. For details see https://turbo.hotwired.dev/handbook/building#opting-out-of-caching

Perhaps in the future Flowbite can react to turbo:before-cache or [data-turbo-preview], perhaps even just by providing some documentation for typical use cases. (Modals via turbo-frame seems to be one of those.)

@daniel-rikowski it worked, but I think that this is not something truly related to cache itself.

I have a custom js controller which handle the modals on my app

the controller.js file here:

// controller.js
import { Controller } from "@hotwired/stimulus";
import { Modal } from "flowbite";

export default class extends Controller {
  constructor() {
    super(...arguments);
    this.modal = new Modal(this.element);
    this.destroyMissingBackDrop();
  }

  connect() {
    this.modal.show();
  }

  hide() {
    this.element.removeAttribute("src")
    this.modal.destroy();
  }

  // used for form submission as turbo:submit-end->modal--component#submitEnd
  submitEnd(e) {
    if (e.detail.success) {
      this.hide();
    }
  }

  destroyMissingBackDrop() {
    document.querySelector("[modal-backdrop]")?.remove();
  }

}

the HTML file here:

<%= tag.turbo_frame(id: :modal) do %>
  <div data-controller="modal--component" tabindex="-1" aria-hidden="true" class="fixed top-0 left-0 right-0 z-50 hidden w-full p-4 overflow-x-hidden overflow-y-auto md:inset-0 h-[calc(100%-1rem)] max-h-full">
    <div class="relative w-full max-w-md max-h-full">
      <!-- Modal content -->
      <div class="relative bg-white rounded-lg shadow dark:bg-gray-700">
        <button type="button" class="absolute top-3 right-2.5 text-gray-400 bg-transparent hover:bg-gray-200 hover:text-gray-900 rounded-lg text-sm w-8 h-8 ml-auto inline-flex justify-center items-center dark:hover:bg-gray-600 dark:hover:text-white" data-action="click->modal--component#hide">
          <svg class="w-3 h-3" aria-hidden="true" xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 14 14">
            <path stroke="currentColor" stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="m1 1 6 6m0 0 6 6M7 7l6-6M7 7l-6 6"/>
          </svg>
          <span class="sr-only">Close modal</span>
        </button>
        <div class="px-6 py-6 lg:px-8">
          <h3 class="mb-4 text-xl font-medium text-gray-900 dark:text-white"><%= title %></h3>
          <%= body %>
        </div>
      </div>
    </div>
  </div>
<% end %>

I had to post the code to say that when the user clicks on the the button to close the modal it works perfectly, but when the user clicks on the backdrop or press Esc to close the modal it will behave like the video I posted here

I tried to find a solution to handle the click on the backdrop element on my js file but without success so far.

@secretpray
Copy link

secretpray commented Mar 23, 2024

When I open a modal on the route app/students then navigate to the route app/parents and go back to the app/students route the modal that I have opened before blinks on the screen.

I had the same problem. I solved it this way

import { Controller } from "@hotwired/stimulus"

// Connects to data-controller="modals"
export default class extends Controller {
  static values = { 
    modalId: { type: String, default: "modal"},
  }

  initialize() {
    this.#addCacheControlMetaTag()
  }

  connect() {
    this.showModal()
  }

  disconnect() {
    this.destroyModal()
    this.#removeCacheControlMetaTag()
  }

  closeModalOnEscape(event) {
    if (event.key === "Escape") {
      this.closeModal(event);
    }
  }

  showModal() {
    this.element.classList.remove("hidden")
    this.element.classList.add("flex")
    this.modalBackdrop.classList.remove("hidden")
    this.bodyElement.classList.add("overflow-hidden")
  }

  destroyModal() {
    if (this.element) {
      this.element.classList.add("hidden")
      this.element.classList.remove("flex")
    }
    if (this.modalBackdrop) this.modalBackdrop.classList.add("hidden")
    this.bodyElement.classList.remove("overflow-hidden")
    
    if (this.turboFrame) {
      this.turboFrame.src = ""
      this.turboFrame.removeAttribute("complete")    
    }
  }

  closeModal(event) {
    event.preventDefault()
    this.destroyModal() 
  }

  disconnect() {
    this.destroyModal()
  }

  get modalBackdrop() {
    return document.querySelector("#modalBackdrop")
  }

  get bodyElement() {
    return document.querySelector("body")
  }

  get turboFrame() {
    return document.querySelector(`turbo-frame#${this.modalIdValue}`)
  }

  #addCacheControlMetaTag() {
    const meta = document.querySelector('meta[name="turbo-cache-control"]')

    if (!meta) {
      Turbo.cache.exemptPageFromCache()
      // or
      // const meta = document.createElement('meta')
      // meta.name = 'turbo-cache-control'
      // meta.content = 'no-cache'
      // document.head.appendChild(meta)
    }
  }

  #removeCacheControlMetaTag() {
    document.querySelectorAll('meta[name="turbo-cache-control"][content="no-cache"]').forEach(el => el.remove());
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants