8000 Lazy load default locales by lacco · Pull Request #865 · countries/countries · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Lazy load default locales #865

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 29, 2024
Merged

Lazy load default locales #865

merged 3 commits into from
Sep 29, 2024

Conversation

lacco
Copy link
Contributor
@lacco lacco commented Sep 6, 2024 •
edited
Loading

While analyzing my project's load times, I realized that the country initializer took almost a second:

start = Time.now;

ISO3166.configure do |config|
  config.locales = %i[en fr]
end

puts "Time elapsed #{(Time.now - start) * 1000} ms"

I was able to break it down to this line:

https://github.com/countries/countries/blob/master/lib/countries/configuration.rb#L25

In my project, I18n.available_locales is being set later during the initialization.
Because of that, the default I18n.available_locales are being loaded, which take some time to initialize.

While in most projects I18n.available_locales should be set earlier,
just lazily loading the default locales might be a good improvement anyway.

without lazy default locales: Time elapsed 965.10 ms

with lazy default locales: Time elapsed 0.006 ms

@lacco lacco changed the title lazy load default locales Lazy load default locales Sep 6, 2024
@lacco lacco marked this pull request as ready for review September 6, 2024 07:52
lacco
lacco commented Sep 6, 2024
View reviewed changes
spec/configuration_spec.rb

expect(ISO3166::Configuration.new.locales).to eq([:test])
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if those tests are needed.
They helped me to make the changes, but they might not provide long-term value.

Happy to remove them.

@pmor
Copy link
Member
pmor commented Sep 6, 2024

Thanks for the PR!

Sounds like a great improvement, I should have some time next week to look at it.

@lacco
Copy link
Contributor Author
lacco commented Sep 27, 2024

After a recent rails upgrade, this doesn't seem to be an issue anymore 🤷

We can close this PR if we don't see any use of it.

@pmor
Copy link
Member
pmor commented Sep 29, 2024

Last few weeks were really busy but, but this makes 100% sense. No point loading the default locales right away, as the configuration might set a completely different set of locales.

@pmor pmor merged commit 1781e40 into countries:master Sep 29, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lacco @pmor
0