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

Hooks with dynamic ID are not properly destroyed #3651

Closed
SteffenDE opened this issue Jan 24, 2025 · 0 comments · Fixed by #3652
Closed

Hooks with dynamic ID are not properly destroyed #3651

SteffenDE opened this issue Jan 24, 2025 · 0 comments · Fixed by #3652
Labels

Comments

@SteffenDE
Copy link
Collaborator

Originally reported by @Gazler in Discord:

Mix.install([
  {:phoenix_playground, "~> 0.1.6"},
  {:phoenix_live_view, "~> 1.0.0", override: true},
])

defmodule DemoLive do
  use Phoenix.LiveView

  def mount(_params, _session, socket) do
    if connected?(socket) do
      send(self(), :change_id)
    end
    socket =
      socket
      |> assign(id: 1, counter: 0)
      |> push_event("myevent", %{})
    {:ok, socket}
  end

  def handle_info(:change_id, socket) do
    {:noreply, assign(socket, id: 2)}
  end

  def handle_event("lol", _params, socket) do
    {:noreply, socket}
  end

  def handle_event("reload", _params, socket) do
    counter = socket.assigns.counter + 1
    socket =
      socket
      |> push_event("myevent", %{})
      |> assign(counter: counter)

    socket =
      if counter > 4096 do
        push_navigate(socket, to: "/")
      else
        socket
      end

    {:noreply, socket}
  end

  def render(assigns) do
    ~H"""
    <div id="main" phx-hook="OuterHook">
      <div phx-hook="InnerHook" id={"id-#{@id}"} />
      This is an example of nested hooks resulting in a "ghost" element
      that isn't on the DOM, and is never cleaned up. In this specific example
      a timeout is used to show how the number of events being sent to the server
      grows exponentially.
      <p>Doing any of the following things fixes it:</p>
      <ol>
        <li>Setting the `phx-hook` to use a fixed id.</li>
        <li>Removing the `pushEvent` from the OuterHook `mounted` callback.</li>
        <li>Deferring the pushEvent by wrapping it in a setTimeout.</li>
      </ol>
    </div>
    <div>
      To prevent blowing up your computer, the page will reload after 4096 events, which takes ~12 seconds
    </div>
    <div style="color: blue; font-size: 20px" id="counter">Total Event Calls: {@counter}</div>
    <div style="color: red; font-size: 72px" id="notice" phx-update="ignore">I will disappear if the bug is not present.</div>
    <script>
      window.hooks.OuterHook = {
        mounted() {
          this.pushEvent("lol")
        },
      }
      window.hooks.InnerHook = {
        mounted() {
          console.log("MOUNTED", this.el);
          this.handleEvent('myevent', this._handleEvent(this));
        },
        destroyed() {
          document.getElementById("notice").innerHTML = "";
          console.log("DESTROYED", this.el);
        },
        _handleEvent(self) {
          return () => {
            setTimeout(() => {
              console.warn("reloading", self.el);
              self.pushEvent("reload", {})
            }, 1000)
          }
        }
      }
    </script>
    """
  end
end

PhoenixPlayground.start(live: DemoLive, port: 4200)

The hook element has a dynamic ID, but when it changes, the old hook is not cleared, causing events to exponentially increase.

@SteffenDE SteffenDE added the bug label Jan 24, 2025
SteffenDE added a commit that referenced this issue Jan 24, 2025
Fixes #3647.
Fixes #3651.
Relates to: #3591.

Issue #3647 was caused by LV uploads relying on DOM attributes like
data-phx-active-refs="1,2,3" being in the DOM to track uploads. The problem
arises when the upload input is inside a form that is locked due to another,
unrelated change. The following would happen:

1. User clicks on a button to upload a file
2. A hook calls this.uploadTo(), which triggers a validate event and
   locks the form
3. The hook also changes another input in ANOTHER form, which also
   triggers a separate validate event and locks the form
4. The first validate completes, but the attributes are patched to the
   clone of the form, the real DOM does not contain it.
5. LiveView tries to start uploading, but does not find any active files.

This case is special in that the upload input belongs to a separate form
(<input form="form-id">), so it's not the upload input's form that is locked.

The fix for this is to only try to upload when the closest locked element
starting from the upload input is unlocked.

There was a separate problem though: LiveView relied on a separate DOM
patching mechanism when patching cloned trees that did not fully share
the same logic as the default DOMPatch. In this case, it did not merge
data-attributes on elements that are ignored
(phx-update="ignore" / data-phx-update="ignore"), therefore, the first
fix alone would not work.

Now, we use the same patching logic for regular DOM patches and element
unlocks.

This difference in DOM patching logic also caused other issues, notably:
   * #3591
   * #3651
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant