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

report: rename i18n to i18n-formatter, move strings to Util #13933

Merged
merged 4 commits into from
Jan 23, 2023

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Apr 27, 2022

All of renderer/i18n.js is formatting stuff, but it also doubled as a home for strings (including state, applying strings from the LHR). this pr:

  • renames report/renderer/i18n.js to report/renderer/formatter.js
  • moves the strings stuff to Util
  • new function Util.applyStrings which takes care of adding the english fallbacks

@connorjclark connorjclark requested a review from a team as a code owner April 27, 2022 21:36
@connorjclark connorjclark requested review from brendankenny and removed request for a team April 27, 2022 21:36
report/renderer/util.js Outdated Show resolved Hide resolved
@@ -805,6 +819,7 @@ const UIStrings = {
runtimeCustom: 'Custom throttling',
};
Util.UIStrings = UIStrings;
Util.strings = {...UIStrings};
Copy link
Member

Choose a reason for hiding this comment

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

What was the reason we set these "static" variables here rather than in the class declaration? Might be a good comment to add.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case, it's because moving UIStrings declaration to be before the class will be bad for git blame history. I'm not sure that's a particularly useful thing to call out in a comment tho

return i18n;
return {
formatter: Util.formatter,
strings: Util.strings as typeof UIStrings & typeof Util.UIStrings,
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
strings: Util.strings as typeof UIStrings & typeof Util.UIStrings,
strings: Util.strings as (typeof UIStrings & typeof Util.UIStrings),

@brendankenny
Copy link
Member

brendankenny commented Jan 23, 2023

bumping strings up onto Util seems like a good move (and love dropping the type parameterization), but (bikeshed time) could i18n still be called i18n? formatter is kind of a mouthful and especially in the context of html generation makes me think of style formatting rather than localization.

@connorjclark
Copy link
Collaborator Author

bumping strings up onto Util seems like a good move (and love dropping the type parameterization), but (bikeshed time) could i18n still be called i18n? formatter is kind of a mouthful and especially in the context of html generation makes me think of style formatting rather than localization.

the main idea was to disambiguate these files:

image

I guess I'm fine with the property still being i18n, but are you also suggesting that formatter is a bad change for the file name too?

@brendankenny
Copy link
Member

Descriptive but not optimal in this context, maybe? FWIW there's also shared/localization/format.js for further name confusion, which maybe should have been called formatter.js depending on where you fall on the nouns vs verbs thing, but that really only got a name at all because core/lib/i18n/i18n.js had to be split to move some of it to shared/.

@connorjclark connorjclark changed the title report: rename report i18n to formatter, move strings to Util report: rename i18n to i18n-formatter, move strings to Util Jan 23, 2023
@connorjclark connorjclark merged commit 5e17931 into main Jan 23, 2023
@connorjclark connorjclark deleted the mv-i18n-formatter branch January 23, 2023 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants