misc(build): publish dedicated lh-i18n module #10415

paulirish · 2020-03-03T12:06:09Z

connor did most of this in #10148. 😁

The API of the module isn't documented persay, but you can follow it pretty easily from i18n-module.js. Here it is:

0.0.1 is already published: https://www.npmjs.com/package/lh-i18n and reflects current lighthouse master.

cc @vidorteg ... wdyt?

we can rename the global. currently it's window.Lighthouse.i18n. How would you like it?
do you need .d.ts files? I can generate those.

lighthouse-core/lib/i18n/i18n-module.js

connorjclark · 2020-03-03T23:34:21Z

proto/lighthouse-result.proto

@@ -442,6 +442,17 @@ message I18n {

  // The message holding all formatted strings
  RendererFormattedStrings renderer_formatted_strings = 1;
+
+  message MessagePath {


remove changes.

connorjclark · 2020-03-03T23:36:42Z

I may of misunderstood the purpose of this PR. Is this just to show @vidorteg or do you want this to land?

connorjclark · 2020-03-04T00:14:44Z

We'll probably also want to publish the collection script in the same package (https://docs.google.com/document/d/1L6TkT2-42MMQ72ZSBMFwUaq7M6mDgA2X0x8oHHKaV_U/edit?ts=5d945064#heading=h.eat4lelgbel alludes to this)

paulirish · 2020-03-04T07:55:16Z

It's to land.

vidorteg · 2020-03-05T19:05:10Z

build/build-i18n-module.js

+ */
+'use strict';
+
+const browserify = require('browserify');


maybe minify it too?

won't CDT minify?

afaiu security reviewers won't like rolling minified code to chromium

good point I was thinking that any other customers could benefit from the reduced size, but it may be harder to get approval.

vidorteg · 2020-03-05T19:15:05Z

lighthouse-core/lib/i18n/i18n-module.js

+
+module.exports = {
+  swapLocale: require('./swap-locale.js'),
+  ...require('./i18n.js'),


We took a similar approach for our prototype:

grabbing the file, bundling it with browserify and importing it.

But hit a couple a blockers inside i18n.js that required some changes. @christychen126 did a nice work cleaning the library and leaving it in a "minimal required" state, @paulirish and @connorjclark what would be the best way for us to share this with the Lighthouse team (setting a PR with some changes, design document)?

What were the blockers? can just start w/ sharing the branch, details on what the issue was (problems with bundling, or using it?)

we are creating a CL in the devtools repo that shows the changes we did to the library and how we are consuming it. We will also be adding CDT so that the three teams can agree on the details. @christychen126 will post once it is ready.

The link to the CL: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2090459
Any comments and feedback are appreciated!

thanks, this was useful. i left a comment there

brendankenny · 2020-03-09T22:50:21Z

@paulirish @vidorteg @christychen126 is there a public interface that we're wanting to commit to? It seems like we don't want to expose/guarantee everything in the screenshot in the top PR comment. That would be a great place to start reviewing.

(https://docs.google.com/document/d/1L6TkT2-42MMQ72ZSBMFwUaq7M6mDgA2X0x8oHHKaV_U/edit?ts=5d945064# has some of it but it's broken up into a few places and not clear that usages listed are exhaustive)

We have a few lingering i18n issues to fix (e.g. #9269) and it would be good to make sure any changes we would need to make wouldn't be externally observable/disruptive.

christyycchen · 2020-03-11T19:03:32Z

We'll probably also want to publish the collection script in the same package (https://docs.google.com/document/d/1L6TkT2-42MMQ72ZSBMFwUaq7M6mDgA2X0x8oHHKaV_U/edit?ts=5d945064#heading=h.eat4lelgbel alludes to this)

Hey @connorjclark, we are okay with not having this in the i18n library, since other customer might want the localizability that i18n provides, but not necessary the localization that requires collection scripts. We also did some customization on collectStrings.js and bake-ctc-to-lhl.js to make those work with DevTools, I will put up a CL soon to show what we changed and would appreciate any feedback then :) /cc @vidorteg

christyycchen · 2020-03-11T19:19:59Z

@paulirish @vidorteg @christychen126 is there a public interface that we're wanting to commit to? It seems like we don't want to expose/guarantee everything in the screenshot in the top PR comment. That would be a great place to start reviewing.

(https://docs.google.com/document/d/1L6TkT2-42MMQ72ZSBMFwUaq7M6mDgA2X0x8oHHKaV_U/edit?ts=5d945064# has some of it but it's broken up into a few places and not clear that usages listed are exhaustive)

We have a few lingering i18n issues to fix (e.g. #9269) and it would be good to make sure any changes we would need to make wouldn't be externally observable/disruptive.

Hey @brendankenny, we are still testing and refining how the DevTools consumes i18n library. We will follow up with a draft CL that migrates one module in DevTools to use the i18n library which will make the exhaustive usages clearer! Thank you. cc/ @vidorteg

brendankenny · 2020-04-06T21:52:13Z

I think this one is chilling, @paulirish?

connorjclark · 2020-04-16T00:27:13Z

Related: https://docs.google.com/document/d/1euLSpgTQ041n_yDgjymGP5GkKdhELuNJF-VPdkrtApI/edit#

vidorteg · 2020-07-06T21:05:43Z

With help of the Lighthouse team , Chromium Devtools ingested a standalone version of i18n:
2119671: front_end/third_party review for i18n.js | https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2119671

Thank for all your help and patience answering our questions Lighthouse Team!

connorjclark and others added 9 commits December 26, 2019 17:53

wip

b141527

refresh event

43521a7

script element load

c777b40

Merge remote-tracking branch 'origin/master' into report-swap-locale

f4c54c5

Merge remote-tracking branch 'origin/master' into report-swap-locale

e10970e

Merge branch 'master' into report-swap-locale

933f960

revert viewer changes.

105dbf4

scripts for publishing lh i18n module

5f2e068

tweaks

69c323d

paulirish requested a review from a team as a code owner March 3, 2020 12:06

paulirish requested review from brendankenny and removed request for a team March 3, 2020 12:06

googlebot added the cla: yes label Mar 3, 2020

paulirish changed the title ~~publish dedicated lh-i18n module~~ misc(build): publish dedicated lh-i18n module Mar 3, 2020

devtools-bot assigned brendankenny Mar 3, 2020

devtools-bot added the waiting4reviewer label Mar 3, 2020

connorjclark reviewed Mar 3, 2020

View reviewed changes

lighthouse-core/lib/i18n/i18n-module.js Show resolved Hide resolved

connorjclark reviewed Mar 3, 2020

View reviewed changes

vidorteg reviewed Mar 5, 2020

View reviewed changes

brendankenny added waiting4committer and removed waiting4reviewer labels Apr 6, 2020

paulirish added the chillin ❄ label Apr 6, 2020

paulirish marked this pull request as draft April 16, 2020 06:49

brendankenny closed this Jul 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc(build): publish dedicated lh-i18n module #10415

misc(build): publish dedicated lh-i18n module #10415

paulirish commented Mar 3, 2020 •

edited

Loading

connorjclark Mar 3, 2020

connorjclark commented Mar 3, 2020

connorjclark commented Mar 4, 2020 •

edited

Loading

paulirish commented Mar 4, 2020 via email

vidorteg Mar 5, 2020

connorjclark Mar 5, 2020

connorjclark Mar 5, 2020 •

edited

Loading

vidorteg Mar 5, 2020

vidorteg Mar 5, 2020 •

edited

Loading

connorjclark Mar 5, 2020

vidorteg Mar 5, 2020

christyycchen Mar 5, 2020

connorjclark Mar 5, 2020

brendankenny commented Mar 9, 2020 •

edited

Loading

christyycchen commented Mar 11, 2020

christyycchen commented Mar 11, 2020

brendankenny commented Apr 6, 2020

connorjclark commented Apr 16, 2020

vidorteg commented Jul 6, 2020

misc(build): publish dedicated lh-i18n module #10415

misc(build): publish dedicated lh-i18n module #10415

Conversation

paulirish commented Mar 3, 2020 • edited Loading

Choose a reason for hiding this comment

connorjclark commented Mar 3, 2020

connorjclark commented Mar 4, 2020 • edited Loading

paulirish commented Mar 4, 2020 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Mar 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vidorteg Mar 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Mar 9, 2020 • edited Loading

christyycchen commented Mar 11, 2020

christyycchen commented Mar 11, 2020

brendankenny commented Apr 6, 2020

connorjclark commented Apr 16, 2020

vidorteg commented Jul 6, 2020

paulirish commented Mar 3, 2020 •

edited

Loading

connorjclark commented Mar 4, 2020 •

edited

Loading

connorjclark Mar 5, 2020 •

edited

Loading

vidorteg Mar 5, 2020 •

edited

Loading

brendankenny commented Mar 9, 2020 •

edited

Loading