i18n: use tsc to collect UIStrings #9487

brendankenny · 2019-07-30T22:10:41Z

moves from using esprima to the typescript compiler to extract UIStrings from our files. Discussed this with @exterkamp ahead of time, though this PR grew a little in the telling. Main benefits:

eliminates an aging and seldom updated dependency for one that we keep updated and we already know can parse our source
tsc automatically extracts jsdoc strings including putting @ tags in separate vars, eliminating one parse step and having to keep track of comment start/end positions
automatically does some format corrections, e.g. removes * and starting whitespace from jsdoc strings, joins across newlines, etc
is actually typed :)

also reorganized a bit so the parsing/ast walking is split off in a separate function, allowing testing to be a bit easier (and laying the groundwork for exposing as a utility for third-party plugins, etc, to use if they wish)

lighthouse-core/scripts/i18n/collect-strings.js

brendankenny · 2019-07-30T22:13:45Z

the changes to collectAllStringsInDir are easier to read if you hide whitespace changes, as a lot of it is removing a level of indentation

lighthouse-core/scripts/i18n/collect-strings.js

brendankenny · 2019-07-30T22:20:21Z

lighthouse-core/scripts/i18n/collect-strings.js

    }

-    // @ts-ignore regex just matched


rearranged the ifs above to avoid this

brendankenny · 2019-07-30T22:21:13Z

lighthouse-core/scripts/i18n/collect-strings.js

  createPsuedoLocaleStrings,
-  convertMessageToPlaceholders: convertMessageToCtc,
+  convertMessageToCtc,


it didn't make sense to rename this just for exporting for tests, but missed in the review for #9114 :)

brendankenny · 2019-07-30T22:23:45Z

lighthouse-core/scripts/i18n/collect-strings.js

-    // @ts-ignore - esprima's type definition is supremely lacking
-    const ast = esprima.parse(justUIStrings, {comment: true, range: true});
-
-    for (const stmt of ast.body) {


there's only ever one stmt (if we did our extraction correctly), so no need to loop :)

patrickhulce

I like it! Looks pretty good, I think the dependency change for me mentally moves this script officially from rag-tag hacky solution to realistic code we support 😄

lighthouse-core/scripts/i18n/collect-strings.js

patrickhulce · 2019-07-31T00:42:28Z

lighthouse-core/scripts/i18n/collect-strings.js

+
+    // @ts-ignore - Not part of the public tsc interface yet.
+    const jsDocComments = tsc.getJSDocCommentsAndTags(property);
+    if (jsDocComments.length === 0) throw new Error(`No Description for message "${message}"`);


can we get rid of the message inside computeDescription then and make this the "Missing description comment" one?

can we get rid of the message inside computeDescription then and make this the "Missing description comment" one?

hmm, do you mean like check that either ast.comment or ast.tags is defined here? Or output an empty description from computeDescription and throw here if it's the empty string?

yeah I meant this is a duplicate case and error message of the fallback in computeDescription, could computeDescription accept ast|undefined and do the check there?

could computeDescription accept ast|undefined and do the check there?

got it

lighthouse-core/test/scripts/i18n/collect-strings-test.js

patrickhulce · 2019-07-31T15:47:30Z

eliminates an aging and seldom updated dependency

Kinda crazy that esprima has more than double weekly downloads than typescript still o.O

exterkamp

Love it! 👋 esprima.

lighthouse-core/scripts/i18n/collect-strings.js

exterkamp · 2019-07-31T20:07:02Z

lighthouse-core/scripts/i18n/collect-strings.js

+    for (const tag of ast.tags) {
+      const comment = coerceToSingleLineAndTrim(tag.comment);
+
+      if (tag.tagName.text === 'description') {


Should this throw if we encounter a non-standard tag?

Should this throw if we encounter a non-standard tag?

I think most of the time we don't care, and if someone really wanted to add a different tag to communicate something else, they could. I could see someone in a plugin adding an @const to the strings and then wanting us to still support them with collect-strings. It seems like a corner case, but otoh if someone misspells @description or something it will be picked up by the missing description error.

That's fair. I'm not sure I'm convinced, but I guess there isn't much harm in letting new tags roam free.

lighthouse-core/scripts/i18n/collect-strings.js

exterkamp · 2019-07-31T20:13:11Z

lighthouse-core/scripts/i18n/collect-strings.js

+ * @return {{placeholderName: string, exampleValue: string}}
+ */
+function parseAtExample(rawExample) {
+  const match = rawExample.match(/^{(?<exampleValue>[^}]+)} (?<placeholderName>.+)$/);


So, at this point, the raw example is in the form "{exampleValue} placeholderName"? The @tag is gone? Seems like this wouldn't be the case based on the function description.

So, at this point, the raw example is in the form "{exampleValue} placeholderName"? The @tag is gone? Seems like this wouldn't be the case based on the function description.

good call. I reworded to make it clearer what it was parsing, but also left the @example context so it's not pretending to not be used for that specific purpose.

lighthouse-core/test/scripts/i18n/collect-strings-test.js

exterkamp · 2019-07-31T20:30:35Z

lighthouse-core/test/scripts/i18n/collect-strings-test.js

+    const liveUIStrings = evalJustUIStrings(justUIStrings);
+
+    expect(() => collect.parseUIStrings(justUIStrings, liveUIStrings))
+      .toThrow(/Incorrectly formatted @example/);


Is this error descriptive enough inside a stack trace?

I think it needs more context around it, what message was bad, or what @example text it was iterating over. This would help immediately know what is wrong.

I think it needs more context around it, what message was bad, or what @example text it was iterating over. This would help immediately know what is wrong.

good point. added example string to error message

lighthouse-core/test/scripts/i18n/collect-strings-test.js

exterkamp · 2019-07-31T20:39:12Z

lighthouse-core/scripts/i18n/collect-strings.js


-    if (regexMatches && !exportsUIStrings) {
-      throw new Error('UIStrings defined but not exported');
+    if (!regexMatch) {


Dunno if it's worth it, but can this be a XOR then a ternary on which error to throw? Ignore if it's pointless, but this kind of boolean logic is deceptively simple, but yet hard to follow.

Dunno if it's worth it, but can this be a XOR then a ternary on which error to throw? Ignore if it's pointless, but this kind of boolean logic is deceptively simple, but yet hard to follow.

that sounds more complicated, but I added a comment and an error message tweak to try to make it clearer :)

patrickhulce

LGTM

patrickhulce · 2019-08-01T14:55:15Z

lighthouse-core/scripts/i18n/collect-strings.js

+        const {placeholderName, exampleValue} = parseExampleJsDoc(comment);
+        examples[placeholderName] = exampleValue;
+      } else {
+        throw new Error('bad tag' + tag.tagName.text);


Suggested change

throw new Error('bad tag' + tag.tagName.text);

throw new Error(`Unexpected tagName "@${tag.tagName.text}"`);

Haha, whoops, I threw that in there to see how hard it was to accidentally trigger that case with reasonable typos and then I guess accidentally committed it. Maybe we'll go with throwing in that case after all :)

patrickhulce · 2019-08-01T14:56:48Z

lighthouse-core/scripts/i18n/collect-strings.js

+ */
+function coerceToSingleLineAndTrim(comment = '') {
+  // Line breaks within a jsdoc comment should always be replaceable with a space.
+  return comment.replace(/\n/g, ' ').trim();


FWIW I tend to go with /\n+/g, ' ' if i'm turning into single line in other contexts, not sure if this is different

FWIW I tend to go with /\n+/g, ' ' if i'm turning into single line in other contexts, not sure if this is different

it might be stretching interpreting a string with multiple line breaks in it as still the same jsdoc string but I think it's the only thing that makes sense here, so, sounds good :)

exterkamp

LGTM, my nits were addressed

i18n: use tsc to collect UIStrings

32f85f9

brendankenny requested a review from patrickhulce as a code owner July 30, 2019 22:10

googlebot added the cla: yes label Jul 30, 2019

brendankenny commented Jul 30, 2019

View reviewed changes

lighthouse-core/scripts/i18n/collect-strings.js Show resolved Hide resolved

brendankenny commented Jul 30, 2019

View reviewed changes

brendankenny assigned exterkamp Jul 30, 2019

brendankenny commented Jul 30, 2019

View reviewed changes

patrickhulce reviewed Jul 31, 2019

View reviewed changes

feedback1

0f04990

vercel bot deployed to staging July 31, 2019 19:29 View deployment

exterkamp reviewed Jul 31, 2019

View reviewed changes

feedback2

cd2e063

vercel bot deployed to staging August 1, 2019 01:32 View deployment

patrickhulce approved these changes Aug 1, 2019

View reviewed changes

exterkamp approved these changes Aug 1, 2019

View reviewed changes

more

87f0690

vercel bot deployed to staging August 1, 2019 18:13 View deployment

brendankenny merged commit 252232d into master Aug 1, 2019

brendankenny deleted the tsc-uistrings branch August 1, 2019 18:45

paulirish pushed a commit that referenced this pull request Nov 6, 2019

i18n: use tsc to collect UIStrings (#9487)

01ece8c

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i18n: use tsc to collect UIStrings #9487

i18n: use tsc to collect UIStrings #9487

brendankenny commented Jul 30, 2019 •

edited

Loading

brendankenny commented Jul 30, 2019 •

edited

Loading

brendankenny Jul 30, 2019 •

edited

Loading

brendankenny Jul 30, 2019

brendankenny Jul 30, 2019

patrickhulce left a comment

patrickhulce Jul 31, 2019

brendankenny Jul 31, 2019

patrickhulce Jul 31, 2019

brendankenny Aug 1, 2019

patrickhulce commented Jul 31, 2019

exterkamp left a comment

exterkamp Jul 31, 2019

brendankenny Aug 1, 2019

exterkamp Aug 1, 2019

exterkamp Jul 31, 2019

brendankenny Aug 1, 2019

exterkamp Jul 31, 2019

brendankenny Aug 1, 2019

exterkamp Jul 31, 2019

brendankenny Aug 1, 2019

patrickhulce left a comment

patrickhulce Aug 1, 2019

brendankenny Aug 1, 2019

patrickhulce Aug 1, 2019

brendankenny Aug 1, 2019

exterkamp left a comment

	throw new Error('bad tag' + tag.tagName.text);
	throw new Error(`Unexpected tagName "@${tag.tagName.text}"`);

i18n: use tsc to collect UIStrings #9487

i18n: use tsc to collect UIStrings #9487

Conversation

brendankenny commented Jul 30, 2019 • edited Loading

brendankenny commented Jul 30, 2019 • edited Loading

brendankenny Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a 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

Choose a reason for hiding this comment

patrickhulce commented Jul 31, 2019

exterkamp left a 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

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

Choose a reason for hiding this comment

patrickhulce left a 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

Choose a reason for hiding this comment

exterkamp left a comment

Choose a reason for hiding this comment

brendankenny commented Jul 30, 2019 •

edited

Loading

brendankenny commented Jul 30, 2019 •

edited

Loading

brendankenny Jul 30, 2019 •

edited

Loading