themesberg / flowbite

Open-source UI component library and front-end development framework based on Tailwind CSS
https://flowbite.com
MIT License
7.31k stars 710 forks source link

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

Open daniel-rikowski opened 5 months ago

daniel-rikowski commented 5 months ago

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 https://github.com/themesberg/flowbite/pull/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: https://github.com/themesberg/flowbite/issues/450, https://github.com/themesberg/flowbite/pull/760, https://github.com/themesberg/flowbite/issues/698, https://github.com/themesberg/flowbite/issues/697, https://github.com/themesberg/flowbite/issues/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 https://github.com/themesberg/flowbite/pull/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 https://github.com/themesberg/flowbite/issues/119#issuecomment-1703815359) 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:

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 https://github.com/themesberg/flowbite/pull/141#issuecomment-1925815453 )

As much as https://github.com/themesberg/flowbite/pull/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 (https://github.com/hotwired/turbo/pull/425#issuecomment-1246358996 ๐Ÿ˜Œ)

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 commented 5 months ago

Could this (resolve?) relate to #793 ?

daniel-rikowski commented 5 months ago

I don't think so. More at https://github.com/themesberg/flowbite/issues/793#issuecomment-1927916320

zoltanszogyenyi commented 5 months ago

Hey @daniel-rikowski,

Thanks a lot for the very informative issue and explanations!

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

https://github.com/themesberg/flowbite/commit/3f2ed0acec6767e6ded5a86cbe3651b7ee55f5c4

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 commented 5 months ago

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.

https://github.com/themesberg/flowbite/assets/30217153/79d877ee-9ff6-4759-8929-76971eac3ec9

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

daniel-rikowski commented 5 months ago

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 commented 5 months ago

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 :cry:) 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 commented 5 months ago

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 commented 4 months ago

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 commented 3 months ago

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());
  }
}