Skip to content

Isolated I18n#660

Merged
joelhawksley merged 9 commits intoViewComponent:mainfrom
elia:isolated-i18n
Apr 2, 2021
Merged

Isolated I18n#660
joelhawksley merged 9 commits intoViewComponent:mainfrom
elia:isolated-i18n

Conversation

@elia
Copy link
Contributor

@elia elia commented Mar 8, 2021

Summary

A proof of concept for #552 (comment) with I18n translations that are only accessible from within the component.

There's still some work to do, but if the general idea is ok this PR can replace #552.
One thing to note is that this code has not been tried on any real-world production app.

Other Information

Warming up --------------------------------------
              global     6.194k i/100ms
      global_missing     2.233k i/100ms
    sidecar_absolute     8.213k i/100ms
             sidecar     7.650k i/100ms
     sidecar_missing     1.459k i/100ms
    sidecar_fallback     2.572k i/100ms
Calculating -------------------------------------
              global     62.470k (± 1.7%) i/s -    315.894k in   5.058230s
      global_missing     22.641k (± 2.2%) i/s -    113.883k in   5.032216s
    sidecar_absolute     82.636k (± 2.0%) i/s -    418.863k in   5.070853s
             sidecar     76.282k (± 2.6%) i/s -    382.500k in   5.017815s
     sidecar_missing     14.882k (± 2.2%) i/s -     74.409k in   5.002379s
    sidecar_fallback     27.377k (± 3.2%) i/s -    138.888k in   5.078596s

Comparison:
    sidecar_absolute:    82636.3 i/s
             sidecar:    76281.6 i/s - 1.08x  (± 0.00) slower
              global:    62469.6 i/s - 1.32x  (± 0.00) slower
    sidecar_fallback:    27377.3 i/s - 3.02x  (± 0.00) slower
      global_missing:    22641.5 i/s - 3.65x  (± 0.00) slower
     sidecar_missing:    14882.2 i/s - 5.55x  (± 0.00) slower

@elia elia mentioned this pull request Mar 8, 2021
Copy link
Contributor

@BlakeWilliams BlakeWilliams left a comment

Choose a reason for hiding this comment

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

I had a few comments/questions but so far this is looking great!

Thank you for adding a benchmark too!

Copy link
Contributor Author

@elia elia left a comment

Choose a reason for hiding this comment

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

Just a few comments to address concerns and questions (still didn't had the time to finish up the PR, fix specs, remove debug statements etc.)

@elia elia force-pushed the isolated-i18n branch 3 times, most recently from 5f4e502 to 2fb94b0 Compare March 10, 2021 22:41
@elia elia marked this pull request as ready for review March 10, 2021 22:47
Copy link
Contributor

@BlakeWilliams BlakeWilliams left a comment

Choose a reason for hiding this comment

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

I had a few small questions/comments, but looking good!

docs/index.md Outdated
</div>
```

##### Translations
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we mark this as experimental similar to how we've marked V2 as experimental?

The goal is to include this for V3 but don't want to commit to a specific API/behavior until then.

#
# The provided array of extensions is expected to contain
# strings starting without the "dot", example: `["erb", "haml"]`
def sidecar_files(extensions)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that we want to make this part of the public API just yet. Thoughts on prefixing this with an _ and making it private, or moving it back to the compiler?

<p class="isolated"><%= t("isolated_translation", default: "This won't be used") %></p>
<p class="missing"><%= t("translations_component.title", default: "Not the original value") %></p>
<p class="helpers"><%= helpers.t("translations_component.title", default: "Not the original value") %></p>
<p class="global"><%= ::I18n.t("translations_component.title", default: "Not the original value") %></p>
Copy link
Contributor

Choose a reason for hiding this comment

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

This was a little confusing since I didn't know we had an existing translations file that had a component namespace. Thoughts on using some other namespace that's more general?

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 cleaned it up a bit, hope is more understandable now.

Copy link
Member

@joelhawksley joelhawksley left a comment

Choose a reason for hiding this comment

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

Some docs feedback. Thanks again for your hard work on this feature 😄

@elia elia force-pushed the isolated-i18n branch 4 times, most recently from eaac036 to 35e95e4 Compare March 16, 2021 08:36
Copy link
Contributor Author

@elia elia left a comment

Choose a reason for hiding this comment

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

I set both APIs as experimental, I think _sidecar_files might come useful for other plugins as well, as it encapsulates what is considered a sidecar file by ViewComponent.

<p class="isolated"><%= t("isolated_translation", default: "This won't be used") %></p>
<p class="missing"><%= t("translations_component.title", default: "Not the original value") %></p>
<p class="helpers"><%= helpers.t("translations_component.title", default: "Not the original value") %></p>
<p class="global"><%= ::I18n.t("translations_component.title", default: "Not the original value") %></p>
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 cleaned it up a bit, hope is more understandable now.

@elia elia self-assigned this Mar 16, 2021
docs/index.md Outdated
<p><%= t("hello") %></p>
```

To access global translations, explicitly mention `helper` or `I18n`:
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to have t also access global translations? I think it's a little awkward to re-define t with a local scope and then expect consumers to know when to use the helpers prefix.

@elia elia force-pushed the isolated-i18n branch 2 times, most recently from 218a982 to 90a3bb7 Compare March 16, 2021 22:08
Copy link
Contributor Author

@elia elia left a comment

Choose a reason for hiding this comment

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

@joelhawksley the fallback to the global I18n has been added, I also updated the benchmark to reflect the different scenarios, and a note in the docs to explain how to reach translation keys shadowed by the sidecar file.

@elia elia requested a review from joelhawksley March 16, 2021 22:11
@Spone
Copy link
Collaborator

Spone commented Mar 16, 2021

This is looking great @elia! 🙌

I'm a bit late to the party, but what about using a dot as a way to disambiguate between local and global I18n?

app/components/example_component.yml

en:
  hello: "Hello world!"

To access the local translation:

app/components/example_component.html.erb

<p><%= t(".hello") %></p>

To access a global translation

<%= t("my.global.translation") %>

So if a global translation is shadowed by the sidecar file you just have to drop the dot:

app/components/example_component.html.erb

<%= t("hello") %>

Seems very Railsy to me but maybe I'm missing something?

Copy link
Member

@joelhawksley joelhawksley left a comment

Choose a reason for hiding this comment

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

Just leaving a small docs tweak here. I think @Spone might be on to something with the local syntax proposal 🤔

@elia
Copy link
Contributor Author

elia commented Mar 17, 2021

@Spone @joelhawksley that's surely feasible, although I have some concerns about that:

  1. introducing more complexity, either by copying some code from the Rails own TranslationHelper, the current implementation is super simple, if we want to have it to mimic Rails maybe we should just use Rails and move back to something more like Sidecar translations #552
  2. t(".my-key") already has a meaning in Rails, I'm not too concerned, since I don't expect it to be a very popular feature, but we should probably add instructions on how to get the original behavior
  3. how likely it is to have a key shadowed by the component's translations? my purely speculative guess: not very often

I don't necessarily oppose doing that, but I lean more toward keeping things simple, adding features later, if required.
What are your thoughts?


One of the first things I tried was to include the original TranslationHelper and have it point to the components I18n. That is actually easier said than done, the helper is full of I18n references and the I18n library itself was built to be global.

@Spone
Copy link
Collaborator

Spone commented Mar 18, 2021

I don't necessarily oppose doing that, but I lean more toward keeping things simple, adding features later, if required.

I totally agree. Let's just keep in mind that changing how locale lookup works in a future version of view_component could be a massive breaking change, and migration would be painful in some cases...

For the sake of the argument, I'm taking the point of view of a developer using view_component here, regardless of the internal implementation (whose complexity has obviously to be considered).

t helper lookup behavior

As stated in the docs, let's follow the principle of least surprise. If I read the code of a project using view_component, and I see this anywhere:

t("hello")

knowing Rails conventions on translations lookup, I would expect it to reference the hello key at the root of the global locale file. It would be surprising that this code looks up at the root when in a controller or view, but in the component sidecar locale file if it's in a component class or template.

If I see this:

t(".hello")

I know that a "lazy" lookup is used, so I'll look a the file structure to figure out what the path to the translation key is. Lazy lookup, that can be used in controllers and views, could be likened to a "local lookup".
In the current version of view_component, calling t(".hello") from ExampleComponent will look up en.example_component.hello, so it can actually also be used in components already (but without strict isolation).

Switch between global and local translations

In a component, seeing both helpers.t/I18n.t (for global translations) and t (for local translations) could be surprising, especially because you usually pick the former only when you don't have access to t in the current context (eg. in a decorator, in a service object...). Without reading view_component documentation, I would consider this a lack of consistency in coding style, and not necessarily infer that it triggers a different lookup behavior.

Compare it to t("hello") vs. t(".hello"): it shows a clear intent.

Side note : where to put translations

In some projects, I prefer to put my translations in multiple small files, matching the structure of the project (it's easier to find the key you're looking for). Alternatively, especially when I'll need to request translations from other people, it makes more sense to have them all in config/locales/en.yml.

view_component could enable both solutions with t(".hello") by:

  • looking up the key hello in the sidecar locale file first,
  • then, if not found, look up the namespaced key globally at example_component.hello (current behavior)

But is it still real isolation? 🤔

@elia
Copy link
Contributor Author

elia commented Mar 20, 2021

Is it still real isolation?

Probably doesn't matter, the worst that can happen is that we change the title of this PR 😄

@Spone I've been sitting on your comment for 3 days, pondering how could we possibly do that in a way that makes sense for the consumer of this API and the code that needs to implement it. Below I tried to summarize what we're trying to accomplish and the problems we need to overcome.

Desired features

It seems like what we want is:

  1. having the lazy lookup as the preferred way to get the sidecar translations
  2. have it fall-back on the lazy lookup in Rails
  3. have the sidecar translations unreachable from anywhere else in the application

Current issues

I would add the following as issues, but @BlakeWilliams and @joelhawksley please check if these are things we don't want to worry too much about:

  • try to avoid duplicating existing Rails functionality
    Would be quite easy to copy and paste the TranslationHelper from Rails along with its tests, do a few tweaks and make it work. I'm not how well that would work with:

    • supporting different Rails versions and potentially diverging implementations
    • maintaining all that code and keeping up with changes in Rails
  • acknowledge the fact that the I18n library is global in nature
    I had to find out this about the I18n library at my own expense while working on this PR, in multiple occasions I had to fight the fact that I18n really likes to rely on the ::I18n constant for many things. That's why I ended up using just a backend instead of an isolated I18n instance… because there's no such thing! 😅

Open questions

  • in which direction we want to proceed?
  • on which features we're ready to compromise?
  • how much this can become it's own thing (vs. relying on Rails)?

Especially if we want to use the "lazy lookup" I'd say we need to rely on Rails, which in turn means that we need to keep the sidecar translations global either respecting the virtual_path of the templates or adding our own view_components scope.

I'm interested in your thoughts on this, but also on alternative approaches I might have missed.

@elia
Copy link
Contributor Author

elia commented Mar 22, 2021

@Spone @BlakeWilliams I just pushed an additional commit with an attempt at isolated lazy path translations for components.

I also dropped support for multiple translations using the t/translate helper directly in the component, that can be added back, either now, or down the road. In this case I think we're on a good long term path for improvements (unlike changing from lazy to non-lazy and vice-versa, as pointed out by @Spone), and we're free to ship a minimalistic version initially.

What else can change to have this ready for a merge? / What other criteria should it meet?

@elia elia force-pushed the isolated-i18n branch 2 times, most recently from 16321bc to 1969256 Compare March 24, 2021 19:01
@elia elia requested a review from joelhawksley March 24, 2021 20:45
Copy link
Member

@joelhawksley joelhawksley left a comment

Choose a reason for hiding this comment

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

👋🏻 leaving a naming comment here. I'm going to defer to @BlakeWilliams for the technical review.

docs/index.md Outdated

##### Translations (experimental)

To use experimental support for `I18n` translations, include `ViewComponent::SidecarI18n`:
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about renaming this to Translatable be more idiomatic Ruby?

Suggested change
To use experimental support for `I18n` translations, include `ViewComponent::SidecarI18n`:
To use experimental support for `I18n` translations, include `ViewComponent::Translatable`:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great suggestion, it fits well with existing modules 👍

@joelhawksley
Copy link
Member

@elia I have a small favor to ask you ❤️

Might you be willing to open a separate PR introducing _sidecar_files? I'd love to use it on a PR I'm working on 😄

@elia
Copy link
Contributor Author

elia commented Mar 24, 2021

@joelhawksley sure, will take just a minute 👍

@elia
Copy link
Contributor Author

elia commented Mar 24, 2021

@joelhawksley there you go #676

OT is there a reason for sticking with bundler v1?

@joelhawksley
Copy link
Member

OT is there a reason for sticking with bundler v1?

I think it's due to how many Ruby/Rails versions we support in CI?

Copy link
Contributor

@BlakeWilliams BlakeWilliams left a comment

Choose a reason for hiding this comment

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

I had two small questions, but otherwise this looks great!

@joelhawksley joelhawksley merged commit 605d2aa into ViewComponent:main Apr 2, 2021
@joelhawksley
Copy link
Member

Thanks for your work on this PR @elia!

@elia elia deleted the isolated-i18n branch April 3, 2021 06:00
@elia
Copy link
Contributor Author

elia commented Apr 3, 2021

Thanks for making the final changes and merging!

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.

5 participants