-
Notifications
You must be signed in to change notification settings - Fork 262
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
Enhancement/i18n thread safe #647
Enhancement/i18n thread safe #647
Conversation
This PR is related to #641 :) |
cc @mi-wood again, or @tkwidmer. This seems like a good PR, making sure our internationalization (I18n) is ready for multi-threaded Puma. It even adds tests! I'm personally not very good with Ruby, so I don't think my review would be super meaningful here. If anyone good with Ruby can give this a second pair of eyes, that would be great. Or at least give it the thumbs up so we can approve it, so the author gets credit for this PR toward their |
As rails docs says https://guides.rubyonrails.org/i18n.html#managing-the-locale-across-requests When we use 'I18n.locale =' the current locale could leak into following requests this is a standard and recommended way to deal with it :)
de330a6
to
7b22422
Compare
@brunoocasali thank you for rebasing! I have simply been backed up with the number of pull requests we got this month, plus other life things. (I'm trying to work through the remaining pull requests. The fewer of them that are left, the easier reviewing each one becomes, due to not having to worry which one will have merge conflicts, or which one will cause the next one to be harder to test.) I am going to ensure that all the helpful Pull Requests we have received are either "Approved" or merged by the end of the month. I believe all of your Pull Requests are looking good, and we will be approving and eventually merging them. Thanks for sticking with this as I work through the backlog. |
Co-authored-by: DeeDeeG <[email protected]>
I do recall the documentation for I18n saying this is the correct way. If there are no issues in manual testing, then I am inclined to merge this. |
Okay, this works. I tested by going to about:preferences in Firefox, updating the "preferred language for displaying pages", then visiting links/refreshing the page at https://refuge-deedeeg-test.herokuapp.com. My "preferred language" was always respected accurately. (As I understand it, this will only really make a difference if/when we enable multiple workers for Puma some time in the future. Note to other maintainers/readers: see #641 for discussion about multiple workers in Puma. May or may not be merged soon, depending on how review/discussion goes.) I then compared page load times between:
I didn't notice a significant difference; either one would be faster or slower seemingly at random. So this pull request does not appear to have any adverse impact on performance. I'll watch this change on the staging instance, after merging, and hopefully it will continue to behave well as intended in the coming days. In my opinion, this is ready to merge. |
shared_examples_for 'localized request' do |action| | ||
it 'calls I18n.with_locale in requests' do | ||
allow(I18n).to receive(:with_locale) | ||
|
||
get action | ||
|
||
expect(I18n).to have_received(:with_locale).with(:en) | ||
end | ||
end |
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.
@brunoocasali would it be very difficult to also test loading a page with another locale? So far our test suite runs strictly against the site in English. I would love to have some test for a locale other than English, even if it was a very basic or simple test.
If you could do that, it would be much appreciated. If it's a lot of work (or really if you prefer not to for any reason), let me know, and I'll still accept this pull request without it.
Thank you, and best regards.
- DeeDeeg
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.
Hi @DeeDeeG, thanks for your review!
Don't you think that way we could start testing some code that are already tested by the framework?
The test in this PR just guarantees that every request will contain some kind of language definition.
When we add a multi-language switcher like a querystring called "lang" for example we could add another spec here, to check:
If the url contains the "lang" querystring all the messages will be translated properly.
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.
It should be fairly easy to set the locale within a spec's code.
But I can see how that would be unrelated to this pull request. Perhaps when I re-add the language switcher feature, I will @mention
you to see if you might be interested in adding some tests. No obligation, but certainly it would be appreciated if you wanted to help with that.
Thanks for this, I'm going to merge now.
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.
@DeeDeeG of course! just mention me and I see what can I do ;)
Yeah, this change will not improve the performance or impact in a negative way either. This just makes sure that there is no way someone will request a page in "pt-BR" and get the answer in "fr" because someone requested nanosecods before :) |
* Finish Upgrading Webpacker to v5 (#637) - Gemfile[.lock]: Update webpacker to 5.2.1 - Webpacker: Update config files Ran "rails webpacker:install" and committed the changes. In "config/webpacker.yml," kept the ".js.erb" entry under "extensions:" so that "app/javascript/packs/lib/maps.js.erb" is included. In "config/webpack/environment.js," kept jQuery and rails-erb-loader configs, as we are still using these. - JS dependencies: Upgrade webpack-dev-server Commit the changes from running `rails webpacker:install`, but using a caret "^" semver range for "@rails/webpacker", rather than an exact version. (So that we get updates in the 5.x series) * Update dependencies for September 2020 (#636) - dependencies: Remove `swagger` node module We don't actually use this. Also removes 200 sub-dependencies, and makes our yarn.lock file about 1270 lines shorter! - yarn.lock: Upgrade bootstrap to 4.5.2 (Was at version 4.4.1) - dependencies: Resolve node-fetch to ^2.6.1 - yarn.lock: Upgrade "selfsigned" and "node-forge" - Gemfile[.lock]: Update Rails and dependencies rails was 5.2.4.3, now it's version 5.2.4.4 * Keeping filters state on pagination (#638) - Keeping filters state on pagination - Fix Code Climates complaints. - Fixed active issue on ada filter buttons - Fixed undefined function problem when run rspec. * Add rubocop and resolve lint errors (#644) - Initialize rubocop - Run rubocop with --fix - Don't require magic comment This will affect every file and have potential side effects, so I'm going to start with this turned off. - Resolve lint errors in `app` - RSpec linting - Resolve remaining - Fix wrong change - Singleton method for verify - New lint rules - Add rubocop to travis config - Correct docker compose command - Check for geo The condition was actually supposed to be `if geo = results.first`, because that's not always obvious the intention and fails lint, I'm just doing a truthy check for it which should be the same. - Fix typo - Add contributing docs for rubocop * Applying ADA filters for the Map view of search. (#651) - Applying ADA filters for the Map view of search. - Changed comment for polyfill URLSearchParams - Fixed bug of map marker content not showing. * Add a standard EditorConfig configuration for every contributor to follow (#649) * Enhancement/i18n thread safe (#647) - Provide a way to respond requests without thread-safe issues As rails docs says https://guides.rubyonrails.org/i18n.html#managing-the-locale-across-requests When we use 'I18n.locale =' the current locale could leak into following requests this is a standard and recommended way to deal with it :) - Create a shared_example to test I18n locale switching - Update spec/controllers/pages_controller_spec.rb * Upgrade puma to latest version 5.x (#641) * yarn.lock: Update "http-proxy" to v1.18.1 (fe195d7) Co-authored-by: Lucas <[email protected]> Co-authored-by: Tegan Rauh <[email protected]> Co-authored-by: Bruno Casali <[email protected]>
Context
This code makes any request free of locale leaking problems.
According to the rails guides: https://guides.rubyonrails.org/i18n.html#managing-the-locale-across-requests
Using
I18n.locale
is not a recommended way to set current locale for a request, using thewith_locale
is the best option!Btw, searching more about the
http_accept_language
I've found this iain/http_accept_language#68Summary of Changes
I18n.locale
Checklist