new_audit(fr): uses-responsive-images-snapshot

[adamraine]

As discussed in #12684, we will add snapshot support to ByteEfficiency audits by creating a separate audit and sharing some functionality between the two files.

[adamraine]

Is there a decent way to share UI strings between files without putting them in i18n.UIStrings? This is pretty sketchy, I'm not certain the imported strings will even be translated.

[connorjclark]

They would be translated just the same. The collect-strings script requires each module, so L26 is actually executed as you'd expect.

This is novel, but seems good to me. None of these strings might be better suited for lighthouse-core/lib/i18n/i18n.js ?

[adamraine]

No, we are only using title and description which are specific to these two files.

[brendankenny]

They'll be translated, but because they're in UIStrings in uses-responsive-images.js, not in here. collect-strings does require the module, but only for getting each string value, not for getting the list of strings to include (that comes from tsc parsing of the regex-extracted local UIStrings object literal).

However, title and description still won't ever be localized because __filename in this file results in ids like .../uses-responsive-images-snapshot.js | title and won't match .../uses-responsive-images.js | title, which is what's in the locale files, so it'll always fall back to the english string added in the Object.assign() call.

There's not really a pretty way of doing this. We wanted strings to be defined in the file where they are used and we wanted to not have to come up with globally-unique names, so this is one of the constraints.

I'm not sure if we want to go down this path and it may be opening pandora's box just by mentioning this, but....there's no reason why the first argument of i18n.createMessageInstanceIdFn() has to be __filename. It could also be const str_ = i18n.createMessageInstanceIdFn(require.resolve('./uses-responsive-images.js'), UsesResponsiveImages.UIStrings) and all the strings could be over in the original file. Or you could have two str_s, one for local strings and one for the other file's.

Please consider that these might be terrible ideas before trying them, though :) It doesn't seem obviously dangerous, but it does e.g. start to introduce shadow file dependencies that are much harder to analyze and refactor than the normal require/import ones (and we don't currently have any test coverage for making sure all audits are translated, just that they're translatable as long as they're following the same pattern as the rest). If it gets complicated it may be a major pain to unwind.

edit: fixed the i18n.createMessageInstanceIdFn() call

[adamraine]

I've changed the implementation to what @brendankenny is talking about. The UI strings are translated as we expect. I also tried to highlight the new dependency by importing the str_ function from UsesResponsiveImages.

[brendankenny]

Nice! It still feels a little weird, but str_ is designed to provide portable results (within reason) and this is a nice way to keep the dependencies explicit.