Lighthouse: Double-Dollar assertion is overly aggressive

[exterkamp]

The double dollar assertion: https://github.com/GoogleChrome/lighthouse/blob/e87a8b76e66208be0266c6e3a3b648dbebe79521/lighthouse-core/scripts/i18n/collect-strings.js#L344

Is a bit aggressive, it should check for basic $$ but allow $ICU_0$$ICU_1$. This should be validated with regex like \$([^$]*?)\$ and asserting that each capture group isn't empty instead of a more basic \$\$.

[connorjclark]

"good first issue" what is this even

[patrickhulce]

Wait yeah if Connor and I both have no clue what this is talking about this is gotta be "good shane-only issue" ;)

[piotrzarycki]

soo, it's possible to take care of it? :P

[connorjclark]

@piotrzarycki go for it. work involved is indeed "good first issue"-level assuming it is just doing what Shane said. it's just the motivation that is opaque

[exterkamp]

Sorry. From the Chromium slack:

Oh, hm. I think this is an overly aggressive assertion. We are trying to catch "$$" as an escape sequence for "$". So something like "$5.00" being escaped as $$5.00 leads to a problem because that "placeholder" is empty and causes issues. But, as I read the internal comment on this check, it specifically calls out adjacent placeholders as valid like $ICU_0$$ICU_1$ . This is actually an error with the naive check in Lighthouse.

So, this test in Lighthouse is checking for $$ naively, when it really needs to be checking for $$ but not $ICU_0$$ICU_1$ which is valid.