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

refactor: share i18n formatting and locales between core and report #13120

Open
brendankenny opened this issue Sep 24, 2021 · 2 comments
Open
Assignees
Labels
i18n internationalization thangs P3 report

Comments

@brendankenny
Copy link
Member

brendankenny commented Sep 24, 2021

tl;dr: our formatting code + intl-messageformat is relatively small. At the very least we can rearrange it to easily ship with swap-locale (#10148), but we should seriously consider shipping it with the main report as well.

This came up in #10148 (comment) most recently, but also just came up in the context of the flow-report (#13034 (comment)). We've also had a few instances where we've wanted to format strings in the standalone report with replacement values and haven't been able to.

Starting with concrete plans and moving to more fanciful ideas:

  • move formatting code to a new directory so it can be easily shared by core and report-adjacent code. This solves the tsc issues in report: swap locale, enable in viewer #10148 (comment), and makes the rolluping more straightforward: it's not reaching deep in core for files to bundle and it's clearer from file location that what's imported/required needs to be kept compatible across environments.
    • I have a WIP branch for this here: master...d232f8d
    • The basic layout would be something like
    lighthouse-core/lib/
      - i18n.js (mostly `UIStrings` and `str_` functionality)
    
    shared/localization/
      - format.js (`isIcuMessage`, `getFormatted`, `replaceIcuMessages`, etc)
      - swap-locale.js
      - locales.js
      - locales/*.json
    
    • happy to iterate on any part of this. shared/ is a terrible name so would be happy to get suggestions :)
    • the possibility of a shared/ came up before in the context of where the report-generator should live. It should probably move to shared/ as well.
    • where the i18 readme and the supporting scripts should live relative to all this is an open question. It is unfortunate that it gets more spread out
  • ship report: swap locale, enable in viewer #10148 using the separate directory. Will fix the tsc issues and some of the bundling should be simpler (e.g. no shimming path or lighthouse-logger)
  • Shrink the plain format.js bundle. The swap-locale bundle is already down to 34KiB (10KiB after gzip) just dropping path and lighthouse-logger, should be smaller without the extra str_/UIStrings functionality that will be in a different file now, and will be even smaller with no swap-locale (which means no _.get/_.set, another 4+KiB each)
    • Switching to esmodules means better tree shaking. Wouldn't ditch much code in format.js, but later intl-messageformat versions ship ESM builds. They are bigger overall, but that means they can be tree shaken, so it's possibly a win.
    • if we do update, will have to deal with Curly braces should be escaped by single quotes formatjs/formatjs#1437. Apparently single quotes are supposed to escape curly braces in the ICU strings. We have two strings that have '{replacement}' and will have to be updated and/or dealt with so old LHRs aren't broken
  • Ship with flow-report. No reason to artificially limit the text we want to show. Even if we only got it down to 25KiB (w/o gzip) that's tiny compared to a series of LHRs :)
  • Ship with the standalone report for the same reason.
    • Next-gen renderFormattedStrings that support replacement.
@brendankenny
Copy link
Member Author

for next steps: happy to open a WIP PR for master...shared-i18n for more concrete discussion, or we can save for the next eng sync

@connorjclark
Copy link
Collaborator

Possible dir structure:

  • Move lighthouse-core/lib to lib
  • make new folders in lib based on runtime env: lib/shared, lib/node, etc
  • could also move lighthouse-logger to lib/shared/lighthouse-logger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n internationalization thangs P3 report
Projects
None yet
Development

No branches or pull requests

4 participants