elixir-gettext / gettext

Internationalization and localization support for Elixir.
https://hexdocs.pm/gettext
469 stars 87 forks source link

Remove compile time depedency of Gettext backend module #330

Closed maennchen closed 2 months ago

maennchen commented 1 year ago

Continued discussion of https://github.com/elixir-gettext/gettext/issues/317#issuecomment-1356450733

Removing the compile time dependency to the gettext backend module could speed up compile performance of larger apps considerably.

This has an even bigger impact when used together with ex_cldr which also recompiles if it is linked to the gettext backend. (Setting gettext in configuration - https://hexdocs.pm/ex_cldr/1.6.4/readme.html#configuration)

maennchen commented 1 year ago

@josevalim

[...] does not change the code generated at compile-time

Is that technically correct? We're using Gettext.Interpolation.compile_interpolate/3 to interpolate bindings known at the module's compile time. Therefore the compilation of the module using a translation can depend on the specific compilation string if I understand it correctly.

However: It could make sense to disable compile_interpolate/3 and only use runtime_interpolate/2 instead in the dev / test env. That way, we would get no compile dependency in the environments with frequent changes and the current fast precompiled translations in prod. (I would not decide based on the Mix.env() but rather add a setting to the gettext configuration.)

maennchen commented 1 year ago

Actually ignore my last comment. We’re not considering the usage of the translation / given bindings. We’re just optimizing the use case where all bindings were correctly supplied.

maennchen commented 1 year ago

One solution to the problem I can think of:

One problem with that is approach is that the event do not expose the raw arguments (only the arity). It seems like that could be adapted if we wanted to. I did a small POC some time ago that uses a compile tracer for extraction. (Unfortunately I can't locate it anymore.) With a few small changes to the compile tracing I was able to get it running.

The reason why I'm thinking about that approach is how elixir handles recompilation:

Given the following code, touching priv/test will compile the parent as well:

defmodule CompileTest do
  IO.inspect "Compiling outer"
  defmodule CompileTest.Inner do
    IO.inspect "Compiling inner"
    @external_resource Application.app_dir(:compile_test, "priv/test")
  end  
end

This also applies if the modules are defined next to each other in the same file.

josevalim commented 1 year ago

Compilation tracers do not receive arguments. Plus the macro is important to enforce some properties about the translation. The only way I can think to solve this is to indeed decouple the backend that stores the translations from the front-end and somehow hide this dependency from Elixir - but it is not fully clear how to do so yet.

maennchen commented 2 months ago

Since it is brought up in #389 again, her's my latest thoughts / idea about the topic:

use ing the applications gettext backend directly always has to create a compile time dependency. But we would like an export dependency instead.

We could achieve this by changing how you get translations into a module.

How it looks now:

defmodule MyApp.Gettext do
  use Gettext, ...
end

defmodule MyApp.Greeter do
  import MyApp.Gettext

  def say_hello, do: gettext("hello")
end

What I propose:

defmodule MyApp.Gettext do
  use Gettext, ...
end

defmodule MyApp.Greeter do
  use Gettext, backend: MyApp.Gettext

  def say_hello, do: gettext("hello")
end

The "traditional" way could be deprecated and still be supported for a while to make transition simpler.

josevalim commented 2 months ago

Yes, I like this direction!

josevalim commented 2 months ago

Would you send a PR or is this something you would like me to explore? We should update Phoenix generators too once a new version is out.

whatyouhide commented 2 months ago

In this way the module that uses Gettext (and imports translation funs) wouldn't need to be recompiled if the backend changes?

whatyouhide commented 2 months ago

Btw I also have bandwidth to work on this.

maennchen commented 2 months ago

This has been bugging me for a long time, so I'd be happy to provide a PR sometime (probably earliest sometime in september). But I'm also very happy to yield to @whatyouhide if you have time right now.

In this way the module that uses Gettext (and imports translation funs) wouldn't need to be recompiled if the backend changes?

If we do it correctly: yes.

Right now the backend provides macros for the translations (needed for the extraction). To use it, it has to be required at least, which will mean compile time depdendencies.

Afterwards the backend would only have to provide functions instead of macros. the use Gettext, backend: Backend would make sure that there's macros around which will call the backend function at runtime which should either be just an alias (runtime) or an export dependency.

I have seen some places which used something like this: use SomeModule, option: Module.concat(["SomeOtherModule"]) to avoid a compile time dependency. I hope that won't be needed here since it's quite ugly frankly.

maennchen commented 2 months ago

One nice little side effect of this change:

It would potentially be feasible to do use Gettext, backend: Application.compile_env(:app, [:gettext_backend]) in libraries which would allow to extract to the applications Gettext backend.

whatyouhide commented 2 months ago

I will give this a try in the next few days and otherwise @maennchen you can pick it up later on 🙃

josevalim commented 2 months ago

We only need to do this:

defmacro __using__(opts) do
    opts =
      if Macro.quoted_literal?(opts) do
        Macro.prewalk(opts, &expand_alias(&1, __CALLER__))
      else
        opts
      end

where:

  defp expand_alias({:__aliases__, _, _} = alias, env),
    do: Macro.expand(alias, %{env | function: {:__gettext__, 1}})

  defp expand_alias(other, _env), do: other

and then make sure the backend is not used anywhere at compile-time, then we should be good to go.

My only complaint is that use Gettext is already used to define backends. So my suggestion is this:

use Gettext will be overloaded for a while, but we can do it with backwards compatibility based on the :backend key. If the backend key is not given, then it is deprecated, and you should use Gettext.Backend instead. Also, any use Backend is deprecated.

Gladear commented 2 months ago

@josevalim (or anyone who can answer that) Small I question I can't seem to find the answer too: external dependencies are not subject to the creation of compile-connected dependencies, are they? With the proposed implementation, if I understand correctly, there will be my_app.ex -> (compile) gettext.ex -> (runtime) my_app/gettext_backend.ex. Modifying my_app/gettext_backend.ex won't cause my_app.ex to recompile, right? My bad, just realized it would probably be

my_app.ex
-> (compile) gettext.ew
-> (runtime) my_app/gettext_backend.ex

Forget about that.

On another topic, a small suggestion you may already have in mind, instead of expecting users to type use Gettext, backend: MyApp.Gettext in modules where they need gettext, I think it would be nice to have a clause in MyApp in the scaffolding, allowing to write use MyApp, :gettext instead.

whatyouhide commented 2 months ago

On another topic, a small suggestion you may already have in mind, instead of expecting users to type use Gettext, backend: MyApp.Gettext in modules where they need gettext, I think it would be nice to have a clause in MyApp in the scaffolding, allowing to write use MyApp, :gettext instead.

This is a Phoenix-generator-related thing though, but yeah good callout!

whatyouhide commented 2 months ago

Closed! Woohooo!