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

Remove compile time depedency of Gettext backend module #330

Closed
maennchen opened this issue Dec 20, 2022 · 16 comments
Closed

Remove compile time depedency of Gettext backend module #330

maennchen opened this issue Dec 20, 2022 · 16 comments

Comments

@maennchen
Copy link
Member

Continued discussion of #317 (comment)

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

This comment was marked as outdated.

@maennchen
Copy link
Member Author

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.

@whatyouhide whatyouhide changed the title Remove compile time depedency of Gettext Backend Module Remove compile time depedency of Gettext backend module Dec 23, 2022
@maennchen
Copy link
Member Author

One solution to the problem I can think of:

  • Instead of using macros to extract messages, a compile tracer could be used instead. (imported_function & remote_function)
  • That way the gettext module no longer has to be imported and we can get rid of the compile time depdendency.

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
Copy link
Contributor

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
Copy link
Member Author

maennchen commented Aug 16, 2024

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
Copy link
Contributor

Yes, I like this direction!

@josevalim
Copy link
Contributor

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
Copy link
Contributor

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

@whatyouhide
Copy link
Contributor

Btw I also have bandwidth to work on this.

@maennchen
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

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

@josevalim
Copy link
Contributor

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.Backend, otp_app: ... will be used to define new backends (we should do this first)
  • use Gettext, backend: ... will be how you use a backend

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
Copy link
Contributor

Gladear commented Aug 17, 2024

@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
Copy link
Contributor

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
Copy link
Contributor

Closed! Woohooo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants