mathieuprog / tz

Time zone support for Elixir
Apache License 2.0
125 stars 11 forks source link

Don't set module constant from app env #2

Closed jfcloutier closed 3 years ago

jfcloutier commented 3 years ago

Having a module constant set from the environment at compile time is problematic if the runtime environment is different

mathieuprog commented 3 years ago

Thank you. How could the value for such config be different at runtime than it was at compile time?

jfcloutier commented 3 years ago

Quoting from Be Careful When Using Elixir’s Module Attributes,

Retrieving constants from mix configuration can also cause issues. The following is a popular pattern:

defmodule SomeModule do
@some_constant Application.get_env(:some_app, :some_setting)
...
end

_This seems okay as long as you aren’t changing application settings at runtime using Application.putenv. However, in some cases (particularly when working with umbrella projects), a change to a configuration file will fail to force a recompile of the module. The module then continues to use the old config value.

This can lead to some really confusing problems that are only solved by doing a full rebuild of the project. This problem is most painful when encountered in a third party library.

jfcloutier commented 3 years ago

BTW I very much like the approach you took of compiling the timezone periods into a module instead of building a database :-)

mathieuprog commented 3 years ago

@jfcloutier the article mentions it is discouraged to affect constants a configuration value because:

For the first point, there is no reason to change :reject_time_zone_periods_before_year at runtime.

For the latter, this was a bug and had been resolved in 2019, while the article was written in 2018: https://elixirforum.com/t/mix-will-not-recompile-the-app-when-change-the-config-exs-in-umbrella/11011/15

If you see any other valid reason, let me know:)