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

Configurable backend #519

Merged
merged 2 commits into from
Nov 1, 2018
Merged

Conversation

sbungartz
Copy link

Hi,
this PR addresses issues #428 and #344.
Only supporting the Simple backend to generate translations.js makes sense to me, since a dynamic backend like a database or something custom would raise questions about when to re-build the asset.

If you, like us, use a Chain backend with some custom stuff followed by the Simple backend and are fine with the custom stuff not being present in JS, then you can use the monkey-patch from #428.

This PR would allow to set I18n::JS.backend = I18n::Backend::Simple.new in an initializer, without having to monkey-patch the entire I18n::JS.translate method.

Quickly grepping through the project for backend revealed this other use of it, which might break if just monkey-patching I18n::JS.translate. I think this is a good reason, to allow this to be set from the outside.

Please let me know, what you think of this idea. If you like it, I would be happy to update README, and CHANGELOG and add/update specs. I'm not 100% sure how this might conflict with other features like available_locales.

@PikachuEXE PikachuEXE merged commit 1064920 into fnando:master Nov 1, 2018
@PikachuEXE
Copy link

Released as 3.1.0

@@ -161,7 +170,7 @@ def self.filter(translations, scopes)

# Initialize and return translations
def self.translations
::I18n.backend.instance_eval do
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PikachuEXE Thanks for getting this released so quickly! I just tested it within our app to replace the monkey patch and now it does not load new translations when files are changed in development. This seems to be the case, because Rails calls I18n.backend.reload! for each request, so when the translations are fetched again from I18n::JS.backend, it still gets the old ones, since this instance has not been reloaded.

I was able to work around this in our case using:

I18n::JS.backend = I18n.backend
I18n.backend = I18n::Backend::Chain.new(SomeCustomBackend.new, I18n.backend)

Since Chain#reload! calls reload! on all of its backends, the instance of I18n::JS.backend is also reloaded and everything works.

If one does not use a simple backend at all however, this is not an option.
Would it make sense to actually call reload! here if the backend is initialized?, since it is only called by the sprockets engine when something has changed anyways?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean if I18n::JS.backend = NotSimpleI18nBackend.new
Then I18n.backend.reload, which is called on each request in development
does not reload I18n::JS.backend since I18n::JS.backend != I18n.backend?

I think your way is a proper way to be used for now (for people with I18n::JS.backend != I18n.backend)
Not everyone needs a translation reload on each request (only sprockets users?)
Reloading on each call by default without a way to opt-out seems dangerous to me

I think we can start by improving the doc about using different backends first

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you are probably right. I added some documentation in #521 with possible workarounds.

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

Successfully merging this pull request may close these issues.

2 participants