elixir-cldr / cldr_routes

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

Feature: Language Prefix -> /en/foo; /de/foo #3

Closed rubas closed 2 years ago

rubas commented 2 years ago

Hi @kipcole9

Path parts (the parts between "/") are translated at compile time using Gettext.

Have you considered adding the option of a prefix for a language?

Normally we not only translate the path, we also add prefix to make sure we have no colliding paths. For /login, we would use /en/login and /de/login and so forth.

Without the option to define a default prefix per language, we have to add the prefix to every translation, which is tedious, error prone and convolutes the po files.


localize do
 live("/home", HomeLive)
 live("/validate-account", ConfirmationLive)
 live("/validate-email-update", ConfirmationUpdateLive)
end
msgid "home"
msgstr "de/home"

msgid "settings"
msgstr "de/settings"

msgid "validate-account"
msgstr "de/konto-validieren"
msgid "home"
msgstr "en/home"

msgid "settings"
msgstr "en/settings"

msgid "validate-account"
msgstr "en/validate-account"

and so on.

kipcole9 commented 2 years ago

Its an interesting idea. I have some thoughts and some questions. Priority first is to delivery my ElixirConf EU presentation and then I can get my head back to this. Apologies for being slow to respond, will be back on it over the weekend.

rubas commented 2 years ago

I'm still thinking about the right way to implement this.

Or one could simply go and do

localize do
 live("/_lang_/home", HomeLive)
 live("/_lang_/validate-account", ConfirmationLive)
 live("/_lang_/validate-email-update", ConfirmationUpdateLive)
end
msgid "_lang_"
msgstr "de"
kipcole9 commented 2 years ago

Have been thinking about this too. How about:

localize do
 live("/@locale/home", HomeLive)
 live("/@locale/validate-account", ConfirmationLive)
 live("/@locale/validate-email-update", ConfirmationUpdateLive)
end

And use the "special" but otherwise invalid path segment @locale and I substitute that with the locale name? I still have to fix the issue with path helpers (definitely this weekend, work has been pretty intense these last 10 days).

rubas commented 2 years ago

It looks better and is in the spirit of the assigns but @locale is not an invalid path segment and https://localhost:4000/@locale/home works fine.

We could introduce in the localize macro a @locale (or locale) variable and make sure, it's an invalid path or it doesn't collide with anything, and translate this.

localize do
 live("/#{@locale}/home", HomeLive)
 live("/#{@locale}/validate-account", ConfirmationLive)
 live("/#{@locale}/validate-email-update", ConfirmationUpdateLive)

 # This allows to introduce it `@locale` at any part
 live("/foo/bar/#{@locale}/home", HomeLive)
end

If you only want to cover the case of the classic prefix, a localized_scope is an elegant solution and keeps everything clean.

 live_session :non_auth_user do
    localized_scope "/user/", ElixirWeb do
      localize do
        post("/login", UserSessionController, :create)

        live("/login", User.LoginLive)
        live("/register", User.RegisterLive)
        live("/register/:step", User.RegisterLive)
      end
    end
  end

I still have to fix the issue with path helpers (definitely this weekend, work has been pretty intense these last 10 days).

We are also working on a fix, but one of our repo still has an issue and we are debugging the root case.

kipcole9 commented 2 years ago

Ah, good callout. Definitely not thinking at my best. More coffee required before I get my head back to this.

kipcole9 commented 2 years ago

As of commit 8130861 there is support for interpolating locale, language and territory into a localised route. For example:

  localize do
    get "/#{locale}/locale/pages/:page", PageController, :show
    get "/#{language}/language/pages/:page", PageController, :show
    get "/#{territory}/territory/pages/:page", PageController, :show
  end

Hopefully this implementation meets. your needs. Since it is using standard Elixir interpolation syntax it should be familiar and consistent in its use.

Since path translation needs to occur at compile time (both to generate routes and also because Gettext requires that its arguments are strings), the interpolation support is limited to these three fields. The interpolation is done by splicing the AST in the localize/3 macro.

Feedback and comments welcome. This is the last commit before the release of 0.3.0 in the next day or so.

rubas commented 2 years ago

This looks perfect, but this also generates three distinct localized route helpers.

MarkoWeb.Router.LocalizedHelpers.live_path(conn, UserSessionController, :create) is not matching. Using the locale specific helper (user_session_de_path | user_session_fr_path | ..) doesn't make sense, does it?

I would expect one route helper MarkoWeb.Router.LocalizedHelpers.user_session_path.

Routes

user_session_de_path  POST    /user/de/login          MarkoWeb.UserSessionController :create
user_session_en_path  POST    /user/en/login_en    MarkoWeb.UserSessionController :create
user_session_fr_path  POST    /user/fr/login_fr        MarkoWeb.UserSessionController :create

Router

  live_session :non_auth_user do
    scope "/user/", MarkoWeb do
      localize do
        post("/#{locale}/login", UserSessionController, :create)
      end
    end
  end
kipcole9 commented 2 years ago

Using the locale specific helper (user_session_de_path | user_session_fr_path | ..) doesn't make sense, does it?

Correct, for localised routes it would be expected you would call the specific router on the Helper module (which is what mix phx.routes reports of course).

I would expect one route helper MarkoWeb.Router.LocalizedHelpers.user_session_path

Yes, that is the right expectation.

I will use your example above and dig into it now.

kipcole9 commented 2 years ago

OK, I've added your example to my testing config and I think its working as you would expect.

iex> MyApp.Router.LocalizedHelpers.module_info(:functions)
[
  __info__: 1,
  ...
  user_path: 4,
  user_session_path: 2,
  user_session_path: 3,
  user_session_url: 2,
  user_session_url: 3,
  user_url: 2,
  ...
]

Here's an example:

iex> MyApp.Cldr.put_locale "en"                                                
{:ok, #Cldr.LanguageTag<en [validated]>}
iex> MyApp.Router.LocalizedHelpers.user_session_path %Plug.Conn{}, :create, %{}
"/user/en/login"

iex> MyApp.Cldr.put_locale "de"                                                
{:ok, #Cldr.LanguageTag<de [validated]>}
iex> MyApp.Router.LocalizedHelpers.user_session_path %Plug.Conn{}, :create, %{}
"/user/de/anmeldung"

There is only one function for each base route in LocalizedHelpers. It delegates to the correct specfic helper in Helpers. This approach means I didn't need to fork the Phoenix code or mess with the internals. But it also means that mix phx.routes reports the actual routes and helpers Phoenix knows about. Not the localisable helpers in LocalizedHelpers. I don't current have a solution for this.

I'll revisit the documentation to make this clearer and see if there is any way to create a localized version of the routes mix task, something like mix cldr.routes.

rubas commented 2 years ago

Sorry, I gave you the one example in my routes file that works ;-) The problem is with live routes.

  live_session :default do
    scope "/", MarkoWeb do
      pipe_through([:browser])
      localize do
        live("/#{locale}", HomeLive)
      end
    end
  end

 live_session :non_auth_user do
    scope "/user/", MarkoWeb do
      pipe_through([:browser, :redirect_if_user_is_authenticated])

      localize do
        post("/#{locale}/login", UserSessionController, :create)
      end
    end
  end
iex(15)> MarkoWeb.Router.LocalizedHelpers.module_info(:functions)
[
  __info__: 1,
  helper: 5,
  helper: 6,
  home_live_de_path: 2,
  home_live_de_path: 3,
  home_live_de_url: 2,
  home_live_de_url: 3,
  home_live_en_path: 2,
  home_live_en_path: 3,
  home_live_en_url: 2,
  home_live_en_url: 3,
  home_live_fr_path: 2,
  home_live_fr_path: 3,
  home_live_fr_url: 2,
  home_live_fr_url: 3,
  path: 2,
  raise_route_error: 8,
  static_integrity: 2,
  static_path: 2,
  static_url: 2,
  url: 1,
  user_session_path: 2,
  user_session_path: 3,
  user_session_url: 2,
  user_session_url: 3,
  module_info: 0,
  module_info: 1
]
rubas commented 2 years ago

I'll revisit the documentation to make this clearer and see if there is any way to create a localized version of the routes mix task, something like mix cldr.routes

That would be 👍

@moein-marko - Have a look at it and if you have time, create a PR for the mix command that lists the routes similar to mix phx.routes.

kipcole9 commented 2 years ago

As of commit e2e0b1a I believe the issue with generating singleton helpers for live routes is back on track - but more tests need to be added. Here's where I believe the status is:

I aim to get all of this completed this weekend so you have a release candidate to check against.

rubas commented 2 years ago

Can confirm. LocalizedHelpers work now.


Extracting the translation with interpolation

  live_session :non_auth_user do
    scope "/user/", MarkoWeb do
      localize do
        live("/#{locale}/login", User.LoginLive)
      end
    end
  end

Running mix gettext.extract --merge results in all routes.po files

Having the locale showing up and being translatable is not a bug per se. But you can shoot yourself in the foot, if you translate en with fr for example. And fr and de can be translated in en/routes.po, which make no sense.

If possible, I would prefer if #{locale}, #{language} and #{territory} would not be translatable. But maybe you consider this as a feature?

en/routes.po

[...]
#, elixir-autogen, elixir-format
#: lib/marko_web/router.ex:37
#: lib/marko_web/router.ex:37
#: lib/marko_web/router.ex:37
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:68
msgid "de"
msgstr ""

#, elixir-autogen, elixir-format
#: lib/marko_web/router.ex:37
#: lib/marko_web/router.ex:37
#: lib/marko_web/router.ex:37
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:68
msgid "en"
msgstr ""

#, elixir-autogen, elixir-format
#: lib/marko_web/router.ex:37
#: lib/marko_web/router.ex:37
#: lib/marko_web/router.ex:37
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:68
msgid "fr"
msgstr ""

#, elixir-autogen, elixir-format
#: lib/marko_web/router.ex:68
#: lib/marko_web/router.ex:68
#: lib/marko_web/router.ex:68
msgid "logout"
msgstr ""

#, elixir-autogen, elixir-format
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
#: lib/marko_web/router.ex:49
msgid "new-password"
msgstr ""
kipcole9 commented 2 years ago

Its a good point and I'm in two minds. One of minds is that by the time translation occurs there is no record of the interpolation - that happened earlier on in the process. So perhaps I have a natural bias to consider it a "feature". But perhaps its more likely to be an unexpected side effect?

kipcole9 commented 2 years ago

Closing for now since I think the normal expectation would be that interpolation can happen anywhere in a string. And that the translations would occur after interpolation. Therefore translating before interpolation could produce very unexpected results.