Reload I18n.load_path in development

[kirs]

Right now, when you add new locale file in development, rails server should be restarted to catch new YAML files.

Of course we add new locales not so often, but in huge Rails app single locale is separated by many files, like: billing.en.yml, admin.en.yml, dashboard.en.yml. This patch could be quite useful in this kind of situations.

This patch also solved the case when locale file was deleted, because right now I18n would raise an error.

Generally, if we use reloading for all kinds of resources in development, why don't we use it for locales?

/cc @rafaelfranca @sirupsen

[kaspth]

It feels weird to have both an expanded_paths and an expanded method - why can't we use expanded?

[rafaelfranca]

Yeah, it is a red flag that we have to add this method. We can handle new files inside app without having to change Paths, so I believe we can use the same machinery to load new locales files.

[kirs]

@kaspth

Demo::Application.config.paths["config/locales"].expanded
=> ["/app/config/locales/fr.yml", "/app/config/locales/devise.en.yml", "/app/config/locales/devise_invitable.en.yml", "/app/config/locales/en.yml", "/app/config/locales/dk.yml", "/app/config/locales/simple_form.en.yml"]

expanded returns all available files in config/locales, and for reloading part I just need the absolute path, which would be /app/config/locales in this case

[kaspth]

Thanks for your explanations, that clears things up a lot 👍

[kirs]

Do you have any ideas how to deal with it without adding new methods to the class?

[kaspth]

I can't see it though, we'll probably need new methods in the class. Probably like this:

def extensions
  $1.split(',') if @glob =~ /\{([\S]+)\}/
end

def expand_paths_without_uniq # :nodoc:
  raise "You need to set a path root" unless @root.path

  map { |path| File.expand_path(path, @root.path) }
end

# Expands all paths against the root and return all unique values.
def expanded
  result = []

  expand_paths_without_uniq.each do |p|
    if @glob && File.directory?(path)
      Dir.chdir(path) do
        result.concat(Dir.glob(@glob).map { |file| File.join path, file }.sort)
      end
    else
      result << path
    end
  end

  result.uniq!
  result
end

[kaspth]

This would definitely be cool to have. Though I wonder about the precedent we're setting. As far back as I can remember any change in config/* always meant: restart your app. It wouldn't be too far fetched for users to wonder why we don't reload the other stuff in config.

[egilburg]

Speaking of /config - I always wondered why locale files live in /config (typically reserved for system settings like keys, db adapters, environment/server stuff) and not in /app/locales or /app/assets/locales/ , given that locale files are app/UI/feature specific much more than they are server/config specific.

While not in scope for this PR, perhaps it's worthwhile to explore loading locales from different path and eventually making the default be /app? (just like, historically, all assets used to be in /vendor/assets while now first-party assets are in /app/assets/ ?

This could be a road to making locales a first-class asset, e.g.

• Similar vendoring rules as images/js/css (e.g. 3rd-party vendor/asses/locales vs 1st-party app/assets/locales
• Possibility of built-in minification/compression/CDN like rest of asset pipeline
• Built-in client-side JS exposure (e.g. what right now is done by non-core i18n-js gem)

etc.

[kaspth]

I could see moving locale files to a different directory would make this reloading change easier to understand and remove doubt what's loaded in config.

We'd need more feedback to decide on a change like that though.

[kirs]

@kaspth thanks for feedback! I'm going to fix the implementation part, and the idea about moving locales somewhere else sounds good. Should we invite @dhh to the discussion? 😺

[dhh]

I'd be OK with app/locales if it wasn't a default directory. That is, if you needed it, that's where you'd set it up. It's already a very small minority of apps that need localization, so don't want to trouble app/ with another default directory that only sees rare use.

[kirs]

Though I wonder about the precedent we're setting. As far back as I can remember any change in config/* always meant: restart your app.

@kaspth actually, we already reload config/locales, but only for the changes in existing files: https://github.com/rails/rails/blob/master/activesupport/lib/active_support/i18n_railtie.rb#L56

[kaspth]

Funny, I was working on a feature today that needed localization and just realized this too 😅

Lets keep the locale files where they've always been then 👍

[kaspth]

I still think this should be moved to the path object. Can we find a new method that's stepping stone between the simpleness of expanded_paths and all the extra work expanded does. To me it seems there's code there to be shared - we just need to bring out the similarities more.

[kirs]

Sure we can, I just thought that you wanted to avoid changing Paths class

[kirs]

@kaspth actually, I found different way of dealing with Paths, and we don't need to touch expanded anymore 🚀
For now we only add 2 new methods to Paths: absolute_current and extensions. Is that OK? /cc @rafaelfranca

[kaspth]

@kirs nice 😁

Perhaps we could # :nodoc: the methods to mark them as internal API?

[kaspth]

This Helpers isn't pulling it's weight anymore. Lets just add this as a private class method in the Railtie. We control the methods there, so there's no chance of name collisions if that's what you're worried about 😄

[kaspth]

Aside from the final comments, we can hoist ⚓ and 🚢. Well done, @kirs 👍.

[kirs]

@kaspth all fixed 🚢 thanks for review!

[rafaelfranca]