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

fix: navigate callback running after patch #2736

Merged

Conversation

enewbury
Copy link
Contributor

@enewbury enewbury commented Jul 17, 2023

Fixes issue #2615

Previous behavior

If you navigate from one live view to another, and that liveview immediately does a patch redirect, the navigate will override the patch because it runs its browser history pushes in a browser animation frame that runs on the next repaint.

What changed

This change simply checks if the linkRef has changed by the time the animation frame runs, and if it has, we don't need to run the browser updates. We still call the "onDone" from the withPageLoading wrapper to make sure the events get fired successfully.

image

Image shows that it directs from second page directly to home with the query param set

Test script

Application.put_env(:sample, Example.Endpoint,
  http: [ip: {127, 0, 0, 1}, port: 5001],
  server: true,
  live_view: [signing_salt: "aaaaaaaa"],
  secret_key_base: String.duplicate("a", 64)
)

Mix.install([
  {:plug_cowboy, "~> 2.5"},
  {:jason, "~> 1.0"},
  {:phoenix, "~> 1.7.0"},
  # {:phoenix_live_view, "~> 0.19.0"}
  {:phoenix_live_view, path: "../github.com/enewbury/phoenix_live_view"}
])

defmodule Example.ErrorView do
  def render(template, _), do: Phoenix.Controller.status_message_from_template(template)
end

defmodule Example.HomeLive do
  use Phoenix.LiveView, layout: {__MODULE__, :live}

  use Phoenix.VerifiedRoutes,
    router: Example.Router,
    endpoint: Example.Endpoint

  def mount(_params, _session, socket) do
    {:ok, assign(socket, :value, nil)}
  end

  defp phx_vsn, do: Application.spec(:phoenix, :vsn)
  defp lv_vsn, do: Application.spec(:phoenix_live_view, :vsn)

  def render("live.html", assigns) do
    ~H"""
    <script src={"https://cdn.jsdelivr.net/npm/phoenix@#{phx_vsn()}/priv/static/phoenix.min.js"}></script>
    <script src="http://127.0.0.1:8080/phoenix_live_view.js"></script> 
    <!-- <script src={"https://cdn.jsdelivr.net/npm/phoenix_live_view@#{lv_vsn()}/priv/static/phoenix_live_view.min.js"}></script> -->
    <script>
      let liveSocket = new window.LiveView.LiveSocket("/live", window.Phoenix.Socket)
      liveSocket.connect()
    </script>
    <style>
      * { font-size: 1.1em; }
    </style>
    <%= @inner_content %>
    """
  end

  def render(assigns) do
    ~H"""
    Test Param: <%= @value %>
    <br />
    <.link navigate={~p"/home/second"}>Go to Second page</.link>
    """
  end

  def handle_params(%{"test" => value}, _uri, socket) do
    {:noreply, assign(socket, :value, value)}
  end

  def handle_params(_params, _uri, socket) do
    {:noreply, push_patch(socket, to: ~p"/home?test=1")}
    # {:noreply, socket}
  end
end

defmodule Example.SecondLive do
  use Phoenix.LiveView, layout: {__MODULE__, :live}

  use Phoenix.VerifiedRoutes,
    router: Example.Router,
    endpoint: Example.Endpoint

  defp phx_vsn, do: Application.spec(:phoenix, :vsn)
  defp lv_vsn, do: Application.spec(:phoenix_live_view, :vsn)

  def render("live.html", assigns) do
    ~H"""
    <script src={"https://cdn.jsdelivr.net/npm/phoenix@#{phx_vsn()}/priv/static/phoenix.min.js"}></script>
    <script src="http://127.0.0.1:8080/phoenix_live_view.js"></script> 
    <!-- <script src={"https://cdn.jsdelivr.net/npm/phoenix_live_view@#{lv_vsn()}/priv/static/phoenix_live_view.min.js"}></script> -->
    <script>
      let liveSocket = new window.LiveView.LiveSocket("/live", window.Phoenix.Socket)
      liveSocket.connect()
    </script>
    <style>
      * { font-size: 1.1em; }
    </style>
    <%= @inner_content %>
    """
  end

  def render(assigns) do
    ~H"""
    <.link navigate={~p"/home"}>Home</.link>
    """
  end
end

defmodule Example.Router do
  use Phoenix.Router
  import Phoenix.LiveView.Router

  pipeline :browser do
    plug(:accepts, ["html"])
  end

  scope "/", Example do
    pipe_through(:browser)

    live("/home", HomeLive, :index)
    live("/home/second", SecondLive, :index)
  end
end

defmodule Example.Endpoint do
  use Phoenix.Endpoint, otp_app: :sample
  socket("/live", Phoenix.LiveView.Socket)

  plug(Plug.Parsers,
    parsers: [:urlencoded, :multipart, :json],
    pass: ["*/*"],
    json_decoder: Phoenix.json_library()
  )

  plug(Example.Router)
end

{:ok, _} = Supervisor.start_link([Example.Endpoint], strategy: :one_for_one)
Process.sleep(:infinity)

@@ -407,7 +407,7 @@ export default class LiveSocket {
DOM.findPhxSticky(document).forEach(el => newMainEl.appendChild(el))
this.outgoingMainEl.replaceWith(newMainEl)
this.outgoingMainEl = null
callback && requestAnimationFrame(callback)
callback && requestAnimationFrame(() => callback(linkRef))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternate approach...would it be possible to not call commitPendingLink on line 405 until later in the callback? Possible problems with that would be all the lines above (406-409) get run even if another navigation event is currently taking place, but not sure of a good example of what that would look like.

this.registerNewLocation(window.location)
this.replaceMain(href, flash, (linkRef) => {
if(linkRef == this.linkRef) {
Browser.pushState(linkState, {type: "redirect", id: this.main.id, scroll: scroll}, href)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to worry about possibly losing this scroll position if we don't run this?

@chrismccord
Copy link
Member

This looks great! Can you undo your priv build changes? We don't accept PRs artifacts for security reasons. Thanks!

@enewbury enewbury force-pushed the fix-naviagate-overwrites-patch branch 2 times, most recently from e0194d4 to 86e38c2 Compare July 17, 2023 17:33
@enewbury enewbury force-pushed the fix-naviagate-overwrites-patch branch from 86e38c2 to 32cf1a8 Compare July 17, 2023 17:34
@enewbury
Copy link
Contributor Author

This looks great! Can you undo your priv build changes? We don't accept PRs artifacts for security reasons. Thanks!

Done!

@chrismccord chrismccord merged commit 9f0e3ab into phoenixframework:main Jul 17, 2023
@chrismccord
Copy link
Member

you're my hero! ❤️❤️❤️🐥🔥

@enewbury enewbury deleted the fix-naviagate-overwrites-patch branch July 19, 2023 16:59
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

Successfully merging this pull request may close these issues.

2 participants