elixir-cldr / cldr_routes

Localised routes and path helpers for Phoenix applications
Other
14 stars 6 forks source link

LiveView: "cldr_locale" is not assigned to socket #6

Closed rubas closed 2 years ago

rubas commented 2 years ago

Assigns For each localized path, the Cldr locale is added to the :assigns for the route under the :cldr_locale key. This allows the developer to recognise which locale was used to generate the localized route.

The :cldr_locale key is not added to socket.assigns for LiveView. It works as described for normal controller.

Example

router.ex

  live_session :default do
    scope "/", MarkoWeb do
      localize do
        live("/#{locale}", HomeLive)
        get("/#{locale}/test", HomeController, :test)
      end
    end
  end

home_controller.ex

defmodule MarkoWeb.HomeController do
  use MarkoWeb, :controller

  def test(conn, _opts) do
    IO.inspect(conn.assigns)

    conn
  end
end

home_live.ex

defmodule MarkoWeb.HomeLive do
  use MarkoWeb, :live_view

  @impl Phoenix.LiveView
  def mount(_params, _session, socket) do
    IO.inspect(socket.assigns)

    socket 
  end

  @impl Phoenix.LiveView
  def render(assigns) do
    ~H"""
      <%= gettext("Home") %>
    """
  end

end
kipcole9 commented 2 years ago

My reading of the documentation is that there isn't an :assigns parameter for live routes. So for live routes I put the locale in the :private map instead. Its pretty late my end and just off a long flight so I'll get some sleep and re-check the Phoenix docs, and of course this libs docs as well. Would be better if there is an :assigns map for live routes too so its all more consistent.

kipcole9 commented 2 years ago

Hmmmm, definitely there isn't an :assign option for live route. I haven't spent nearly enough time exploring the live environment. I'd welcome any advice about what needs to go where to make the locale available in the live session.

rubas commented 2 years ago

https://hexdocs.pm/phoenix_live_view/Phoenix.LiveView.Socket.html#t:assigns/0

The data is stored on the Socket and not Conn.

In the live controller we have access to the socket and the session.

https://hexdocs.pm/phoenix_live_view/Phoenix.LiveView.html#c:mount/3

kipcole9 commented 2 years ago

I think this will be tricky to resolve and since its also relevant to the conversation about reconciling the locale derived in Cldr.Plug.SetLocale with the locale recognised by Cldr.Route perhaps this is the best place to bring these together.

Current situation

Discussion topics

  1. Should Cldr.Plug.SetlLocale take a configuration option :assigns meaning that it will set the locale (using Cldr.put_locale/2 if there is a locale set in the assigns. This fits into the current scheme and it means that a priority can be set as to how the locale is set. Its also a straight forward addition.
  2. This doesn't solve the live routes issue. To the best of my knowledge there is no automatic way to propagate the locale to the live view process. This is documented and also discussed in the forum. I think I will need to follow the same approach. Which can be implemented using Cldr.Plug.PutSession to put the locale that is set in the conn into the session.

I suspect (1) above is what you were initially referring to and it makes sense now my head has cleared so I'll go ahead and add that to the Cldr.Plug.SetLocale plug as a :assigns options.

kipcole9 commented 2 years ago

I have published ex_cldr_plugs version 1.1.0 with the following changelog entry:

Enhancements

rubas commented 2 years ago
  1. 👍

2. Live Route Issue

I only checked & noticed, as I haven't integrated it with Cldr.Plug.SetlLocale and was changing the language in the Controller for test purposes. And there I noticed the inconsistency between LiveView und Controller.

Personally (in hindsight 😉), I would not use :assigns and store it :private. For the simple reason to stay consistent between your libraries. That would also help with the misunderstanding between conn.assigns and socket.assigns. You have easy access to :private in the controller too.

Not sure, if you want to introduce this breaking change. In this case, I suggest just adding a note in the documentation that :assigns is only available in conn and if you want the value in LiveView, store in the session (hint to Cldr.Plug.PutSession).


Piecing everything is not as trivial as it could be. I suggest adding another section to the docs about the big picture.

Essentially the "Current Situation" from above. Use this library for route recognition and use Cldr.Plug.SetLocale to set locale.

kipcole9 commented 2 years ago

You raise a really good point about consistency. I hesitated to put both in :private only because my reading is that :private is intended for library use only whereas :assigns forms part of the public API - which was the intent.

I feel like a good next step is to update the docs as you suggest above. And then refocus some effort into an end-to-end Phoenix LiveView app that will help identify the weak points in the I18N flow of an app, You’re feedback is extremely helpful because its been a while since I’ve done that part. It also fits well into the work I’m doing (slowly) in I18n database design (indexing, text search, collation - leveraging ex_cldr_trans).

rubas commented 2 years ago

Proof of Concept

locale_hook.ex

defmodule MarkoWeb.LocaleHook do
  @moduledoc false

  def on_mount(:default, _params, %{"cldr_locale" => locale} = _session, socket) do
    MarkoCldr.put_locale(locale)
    MarkoGettext.put_locale(locale)
    {:cont, socket}
  end

  def on_mount(:default, _params, session, socket), do: {:cont, socket}

  def private(conn, _options) do
    conn
    |> Map.fetch!(:private)
    |> Map.get(:cldr_locale)
    |> Cldr.validate_locale(MarkoCldr)
  end
end

router.ex

    plug(Cldr.Plug.SetLocale,
      apps: [:cldr, :gettext],
      from: [{MarkoWeb.LocaleHook, :private}, :assigns],
      gettext: MarkoGettext,
      cldr: MarkoCldr
    )

    plug(Cldr.Plug.PutSession)

marko_web.ex

  def live_view do
    quote do
      on_mount(MarkoWeb.LocaleHook)

      unquote(view_helpers())
    end
  end

Discussion

:assigns vs :private

When connecting to a live route the ex_cldr is only stored in conn.private - compared to conn.assigns for clasic routes. We talked about the inconsistency already.

Cldr.Plug.SetLocale with from: [:assigns] is not helping and we need to add the same behaviour for :private.

Proposal

Session

LiveView is mounted twice. First as "server-rendering" the classical way and the second time over the socket. [1]

If we stop here, the first mount is with the correct locale. The second mount falls back the default locale.

To restore the locale in LiveView, we need to store in the session and access it later. We don't have access to conn.

plug(Cldr.Plug.PutSession)

Proposal

If from: [:ex_cldr_route] is set in Cldr.Plug.SetLocale, I expect that this is "enough" and the locale will be set at the end.

Either it has to be made pretty clear in the documentation, that plug(Cldr.Plug.PutSession) is needed or it has to be called automatically in this case.

Restore Locale in Liveview

An on_mount hook has to be defined, which restores the locale from the session data. This can be either added manually add in every LiveViewController, with live_session in the router. Or one can hook it up in the foo_web.ex macro.

Proposal

My Opinion

Not sure, how I feel if plug(Cldr.Plug.PutSession) is called implicitly.

I would make sure :private is set always, rename the helper to from: [:ex_cldr_route] and document the fact that session and on_mount hook is needed.

kipcole9 commented 2 years ago

I appreciate the thoughtful feedback and idea and I'm still thinking through what you suggest (and the day job has been a challenge the last couple of days). Will revert ASAP.

kipcole9 commented 2 years ago

I have push commits for the following, accepting your good advice:

Changes based upon your recommendations

Plug.PutSession

I have this as a separate plug because I don't think every app wants to or should store the locale in a session. While the language tag is about 512 bytes in storage terms in the BEAM, there is still a cost to serialise and deserialise it to/from a cookie and transmit it to a client. So I deliberately wanted to make that choice explicit. And there's a case to be made that Cldr.Plug.SetLocale is already doing to much and breaking separation of concerns.

To Do

This leaves the documentation - specifically a guide - to explain how plugs and routes and live view all hang together.

BTW, in the background I am still working on a broader set of documentation on internationising/localising Elixir apps but that's a broader project.

kipcole9 commented 2 years ago

I have added Cldr.Session.put_locale/2 in this commit and moved the conversation to that repo. I will close this issue (but not forgotten the docs still need work)