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

I18nCache: Fix overtaken class var RuntimeError in Ruby 3.0 #1722

Closed
wants to merge 2 commits into from

Conversation

mcls
Copy link

@mcls mcls commented Jan 5, 2021

This fixes errors like the one below in Ruby 3.0 by replacing the class variables with a singleton for caching the translations.

    RuntimeError: class variable @@translate_required_html of SimpleForm::Inputs::StringInput is overtaken by SimpleForm::Inputs::Base
      .../simple_form/lib/simple_form/i18n_cache.rb:13:in `class_variable_get'
      .../simple_form/lib/simple_form/i18n_cache.rb:13:in `get_i18n_cache'
      .../simple_form/lib/simple_form/i18n_cache.rb:8:in `i18n_cache'
      .../simple_form/lib/simple_form/components/labels.rb:9:in `translate_required_html'
      .../simple_form/lib/simple_form/components/labels.rb:71:in `required_label_text'
      .../simple_form/lib/simple_form/components/labels.rb:43:in `label_text'
      .../simple_form/lib/simple_form/components/labels.rb:35:in `label'

Related issue: #1720

This fixes errors like the following in Ruby 3.0:

    RuntimeError: class variable @@translate_required_html of SimpleForm::Inputs::StringInput is overtaken by SimpleForm::Inputs::Base
      .../simple_form/lib/simple_form/i18n_cache.rb:13:in `class_variable_get'
      .../simple_form/lib/simple_form/i18n_cache.rb:13:in `get_i18n_cache'
      .../simple_form/lib/simple_form/i18n_cache.rb:8:in `i18n_cache'
      .../simple_form/lib/simple_form/components/labels.rb:9:in `translate_required_html'
      .../simple_form/lib/simple_form/components/labels.rb:71:in `required_label_text'
      .../simple_form/lib/simple_form/components/labels.rb:43:in `label_text'
      .../simple_form/lib/simple_form/components/labels.rb:35:in `label'
@pedrofurtado
Copy link
Contributor

Pinging maintainers for notification (some of them may be on vacation) 🤝 : @rafaelfranca @tegon @feliperenan @carlosantoniodasilva

@carlosantoniodasilva carlosantoniodasilva self-assigned this Jan 8, 2021
@carlosantoniodasilva
Copy link
Member

Just letting you know that I didn't forget about this, it's on my radar and I plan to work on it in the upcoming days alongside other Ruby 3 / Rails 6.1 upgrades. Thanks for the work here @mcls.

@LeMinh-ThienToan
Copy link

I upgraded to Ruby 3.0.0, Rails 6.1.1 and got same error.

Thanks for the work.

@carlosantoniodasilva
Copy link
Member

@mcls I've been thinking some more about it, and considering dropping that cache entirely instead. It's extra complexity we've had to maintain over the years (and it's just growing a bit now), and I don't believe that cache is such a hot place with SimpleForm to be worth it the complexity.

That's where I'm leaning towards, I'll give it a couple more days. Let me know if you (or anyone else here) have any thoughts in the meantime. Thanks again!

@mcls
Copy link
Author

mcls commented Jan 18, 2021

Hey, thanks for checking. Sounds good to me.

I did a bit of digging and it looks like I18n also has its own cache which people can use if they find it necessary. I also think the default YAML backend is probably performant enough for most cases, given that it stores the translations in memory as hashes.

@carlosantoniodasilva
Copy link
Member

Sounds good, thanks for digging some more there @mcls. I'll get back to it in the next few days and prepare a new release.

@carlosantoniodasilva
Copy link
Member

Closed by 0595088, forgot to reference this one too. Thanks!

carlosantoniodasilva referenced this pull request Jan 20, 2021
I18nCache caused some issues upgrading to Ruby 3, and at the end of the
day I think it's more complexity to keep it around for a marginal gain.

I18n lookups can be expensive, yes, but for most this isn't going to be
a performance hit, and for those where it might be, consider looking at
I18n caching alternatives that aren't specific to these few places where
we had this caching, but to lookups across the board.

Closes #1721
Closes #1720
@matthieua
Copy link

@carlosantoniodasilva Awesome, thanks a lot for taking care of it! Do you think you can make a new release?

@carlosantoniodasilva
Copy link
Member

@matthieua thank you. Yes, definitely, this is on my radar for this week (as well as responders/devise), but I need to make sure #1724 isn't a problem others will hit with SimpleForm when upgrading. (it doesn't appear to be SF specific, but more to investigate there.)

@matthieua
Copy link

@carlosantoniodasilva no problem, that's what I thought, thanks for the update and the awesome work with SF!

@pedrofurtado
Copy link
Contributor

Thanks, @carlosantoniodasilva ! Happy to see a release with ruby 3 support asap 🎉 Thanks again! 🤝

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

Successfully merging this pull request may close these issues.

6 participants