-
Notifications
You must be signed in to change notification settings - Fork 3
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
Configure Rails application i18n configuration in railtie #314
base: main
Are you sure you want to change the base?
Conversation
aac94fc
to
44150f8
Compare
@@ -22,10 +22,17 @@ module Config | |||
OTHER_LOCALE_PATH_REGEX = /#{File::SEPARATOR}(?<locale>[\w-]+)\.(?:yml|rb)/ | |||
REGIONS_LOCALE_PATH_REGEX = /#{File::SEPARATOR}(?<locale>[\w-]+)\.yml/ | |||
|
|||
# Protects against multiple calls to configure_i18n with the same I18n::Config object | |||
@previously_configured_i18ns = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally used a set instead of an array, then learned that sets have surprising behaviour for inclusion by object identity:
(byebug) Worldwide::Config.previously_configured_i18ns.class.name
"Set"
(byebug) Worldwide::Config.previously_configured_i18ns.first === i18n_config
true
(byebug) Worldwide::Config.previously_configured_i18ns.include?(i18n_config)
false
(byebug) Worldwide::Config.previously_configured_i18ns.first.object_id
64480
(byebug) i18n_config.object_id
64480
I believe we're supposed to call Set#compare_by_identity, but there is no guaranteed support on all ruby flavours.
An array is just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 6 out of 7 changed files in this pull request and generated no suggestions.
Files not reviewed (1)
- CHANGELOG.md: Evaluated as low risk
44150f8
to
cf96228
Compare
cf96228
to
0c1030a
Compare
What are you trying to accomplish?
Part of https://github.com/Shopify/address/issues/2796
In the context of a Rails application, call
Worldwide::Config.configure_i18n
with the application's i18n config instance instead of the statically-availableI18n.config
object. Avoid configuring the same i18n config instance more than once.What approach did you choose and why?
Configure Rails application's existing i18n state
Invoke
Worldwide::Config.configure_i18n
in the gem railtie's initializer block, passing in the application'sconfig.i18n
instance.Only configure an i18n config instance once
Maintain a class instance variable in
Worldwide::Config
that remembers each i18n config object that was successfully configured by theconfigure_i18n
method. Subsequent calls toconfigure_i18n
that operate on the same i18n instance will return early.Calling
Worldwide::Config.configure_i18n
with the same i18n config instance is not technically idempotent, as the method also accepts an optionaladditional_components
parameter. This example shows the limitations of my implementation:What should reviewers focus on?
I chose to use
a setan array that remembers previously-seen i18n configs rather than try to manage their state in the context of repeated invocations toconfigure_i18n
. I did not come up with a cheap and clean way to maintain a sane set of updates to i18n configurations, so I went with this simple approach.If someone wants to run
configure_i18n
on an i18n config instance after the first invocation, they can manipulate the cache withWorldwide::Config.previously_configured_i18ns.clear
and take on the responsibility of editing the i18n config instance accordingly.Testing
Railtie testing
In a Rails app, I introduced a railtie that configured the shared i18n configs
available_locales
, ensuring that this railtie's initializer ran before Worldwide's own railie:The railties initialized in the desired order.
Avoid configuring i18n config more than once
In a large app that has both direct and transitive dependencies on Worldwide, with several configuration classes that wind up calling
Worldwide::Config.configure_i18n
on a shared i18n config instance, only the first call toconfigure_i18n
actually updated the i18n config instance.Checklist