[localize] Make placeholder validation pass with different expressions #4530
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: ceb1001 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultsthis-change
render
update
update-reflect
this-change, tip-of-tree, previous-release
render
update
nop-update
this-change, tip-of-tree, previous-release
render
update
this-change, tip-of-tree, previous-release
render
update
update-reflect
|
|
The size of lit-html.js and lit-core.min.js are as expected. |
packages/localize-tools/testdata/build-transform-xliff/input/xliff/es-419.xlf
Show resolved
Hide resolved
packages/localize-tools/src/tests/e2e/placeholder-errors.test.ts
Outdated
Show resolved
Hide resolved
- remove extra param from function that replaces expression - keep expression from before normalization for error logging
augustjk
force-pushed
the
localize-dupe-id-validation
branch
from
5c37cef
to
ceb1001
Compare
augustjk
commented
Jan 31, 2024
Comment on lines
-17
to
-18
| Placeholder error in es-419 localization of changed-expression: unexpected "\${alert("evil") || name}" | ||
| Placeholder error in es-419 localization of changed-expression: missing "\${name}" |
There was a problem hiding this comment.
changed expressions are no longer considered placeholder errors.
| @@ -57,6 +57,9 @@ html`<Hola<b><Mundo & Amigos></b>!>`; | |||
| // Expressions as attribute values should stay as expressions | |||
| html`Hello <b foo=${'World'}>World</b>`; | |||
| html`Hello <b foo=${`Mundo`}>World</b>`; | |||
| html`<b foo=${'Hello'}>Hello</b><b bar=${`Mundo`}>World</b>`; | |||
| html`<b foo=${`Hola`}>Hello</b><b bar=${`Mundo`}>World</b>`; | |||
There was a problem hiding this comment.
This diff is technically unrelated to the changes in the PR but just due to a missing translation target that I noticed from the test file.
Comment on lines
+49
to
+52
| <trans-unit id="s63f0bfacf2c00f6b"> | ||
| <source>Hello</source> | ||
| <target>Hola</target> | ||
| </trans-unit> |
There was a problem hiding this comment.
This is the missing translation target I found, mentioned above.
Comment on lines
+159
to
+164
| const normalizedPlaceholder = normalizeExpressionInTemplateString( | ||
| content.untranslatable | ||
| ); | ||
| const index = remainingProgramPlaceholders.findIndex( | ||
| ({normalized}) => normalized === normalizedPlaceholder | ||
| ); |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #4502
How it used to be before #2405
In the past, the expressions in the translated message placeholder were directly used in generating the output code in transform mode.
foo.ts (source)
es-419.xlf (translation)
used the actual
equiv-text="${user}"value from<target>to generatefoo.js (output)
This meant it was possible for faulty or malicious JS expression to be injected to the output if translation placeholders were modified.
es-419.xlf (translation)
could produce
foo.js (output)
Runtime mode never had this issue since expressions are replaced by numbers in the output anyway and swap them with values given during runtime.
es-419.ts (runtime mode output)
After #2405
#2405 made it so that multiple
msg()calls with different expressions can map to the same translation unit.foo.ts (source)
resulted in a single translation unit.
es-419.xlf (translation)
The value of
equiv-textis based on which expression is encountered first.As such, it also made it so that we never use the expression from translation directly instead always grab the expression from the source code so that we'll produce the appropriate output (both
${user}and${username}in above example). This means strict validation of expressions in placeholders are now unnecessary.However, there remained some validation logic that would check that expressions in placeholders exactly as they are in the source code. If the source code were modified or the order of the template encountered changes while placeholders in the translations are not updated, errors like the follow would show up during build, even though the translations are still perfectly valid to use.
What this PR does
The validation logic is now updated to replace all expressions to
"expr"before comparison so a different expression wouldn't cause it to fail. We still want to make sure the rest of the string matches since placeholders can also include HTML that won't be translated, and placeholders without expressions are still used in output generation for runtime mode.