tests(i18n): no ICU value given to preprocess #9384
Conversation
| @@ -127,9 +127,7 @@ function lookupLocale(locale) { | |||
| * @param {string} icuMessage | |||
| * @param {Record<string, *>} [values] | |||
| */ | |||
| function _preprocessMessageValues(icuMessage, values) { | |||
| if (!values) return; | |||
There was a problem hiding this comment.
feels like this change should be accompanied by a test too if we want to change it here :)
There was a problem hiding this comment.
maybe we should change the typedef below (https://github.com/GoogleChrome/lighthouse/pull/9384/files#diff-4f100f14d281ddfaff2dc4466fc78054R167) to Record<string, any>?
I guess I'm just not sure why
so that when sending no replacement object the function will still fire.
is desirable.
Like why not just if (!values) return {} if we want to convert away from undefined
There was a problem hiding this comment.
I guess I'm just not sure why
so that when sending no replacement object the function will still fire.
is desirable.
Like why not just
if (!values) return {}if we want to convert away fromundefined
I may be misinterpreting your question, but this came up when adding the test. _preprocessMessageValues does the "all ICU replacements in the string have a value provided" check, which works fine if there are multiple replacements to be done and one value (or more) is missing. However if the values object is forgotten completely, this line skips the check, even if the string has replacements that will need values.
It still ends up throwing, just in intl-messageformat, so if we are going to do a check here it made sense to do it for all strings that need replacements (instead of all strings that have values passed in with them).
There was a problem hiding this comment.
Oh, oh, oh, I misread the parsed.elements.forEach line to be Object.values(values).forEach, my bad!
yes, yes this makes perfect sense!
Still would love a test for forgetting the values completely, but I gotcha now! 👍 :)
There was a problem hiding this comment.
Done! I added a new test, and made the error message a bit more helpful.
There was a problem hiding this comment.
LGTM
agree with @patrickhulce that a test for a missing value and a test with the values object missing altogether would be good
Co-Authored-By: Brendan Kenny <[email protected]>
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
Yo @googlebot , I can't add |
Summary
Add test for a missing ICU replacement value when gettingFormatted. Removed early exit from _preprocessMessageValues and added a default so that when sending no replacement object the function will still fire.
fixes: #9383