Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[localize] Fix handling of nested HTML #3674

Merged
merged 3 commits into from Feb 16, 2023
Merged

[localize] Fix handling of nested HTML #3674

merged 3 commits into from Feb 16, 2023

Conversation

aomarks
Copy link
Member

@aomarks aomarks commented Feb 16, 2023

Fixes handling of nested html-tagged template expressions in lit localize analysis.

For example, the following example was analyzed incorrectly:

msg(html`Hello <b>${html`<i>World</i>`}</b>!`);

We combine runs of HTML markup and expressions so that we have fewer placeholders during translation, so we pass the template through an HTML parser, nested expressions and all. The problem was that if the nested expressions contained HTML, those would get parsed as part of the outer message, which would corrupt the analyzed template. The fix was to replace nested expressions with placeholders (the index) during HTML parsing (because we don't need the HTML parser to see the contents of expressions at all) and then substitute the original expression back after HTML parsing.

Fixes #2426

@changeset-bot
Copy link

changeset-bot bot commented Feb 16, 2023

🦋 Changeset detected

Latest commit: 7c836d5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lit/localize-tools Patch

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

@github-actions
Copy link
Contributor

github-actions bot commented Feb 16, 2023

📊 Tachometer Benchmark Results

Summary

nop-update

  • lit-html-kitchen-sink: unsure 🔍 -14% - +3% (-3.76ms - +0.79ms)
    this-change vs tip-of-tree

render

  • lit-element-list: 77.45ms - 81.13ms
  • lit-html-kitchen-sink: unsure 🔍 -4% - +4% (-1.33ms - +1.43ms)
    this-change vs tip-of-tree
  • lit-html-repeat: unsure 🔍 -8% - +2% (-1.01ms - +0.23ms)
    this-change vs tip-of-tree
  • lit-html-template-heavy: unsure 🔍 -2% - +5% (-1.01ms - +2.62ms)
    this-change vs tip-of-tree
  • reactive-element-list: unsure 🔍 -2% - +2% (-1.02ms - +1.22ms)
    this-change vs tip-of-tree

update

  • lit-element-list: 826.58ms - 835.34ms
  • lit-html-kitchen-sink: unsure 🔍 -6% - +3% (-4.90ms - +2.44ms)
    this-change vs tip-of-tree
  • lit-html-repeat: unsure 🔍 -9% - +7% (-30.64ms - +25.10ms)
    this-change vs tip-of-tree
  • lit-html-template-heavy: unsure 🔍 -2% - +1% (-3.12ms - +1.53ms)
    this-change vs tip-of-tree
  • reactive-element-list: unsure 🔍 -0% - +1% (-2.27ms - +9.61ms)
    this-change vs tip-of-tree

update-reflect

  • lit-element-list: 824.64ms - 832.84ms
  • reactive-element-list: unsure 🔍 -0% - +1% (-2.16ms - +10.07ms)
    this-change vs tip-of-tree

Results

lit-element-list

render

VersionAvg timevs
77.45ms - 81.13ms-

update

VersionAvg timevs
826.58ms - 835.34ms-

update-reflect

VersionAvg timevs
824.64ms - 832.84ms-
lit-html-kitchen-sink

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
31.77ms - 33.91ms-unsure 🔍
-4% - +4%
-1.33ms - +1.43ms
unsure 🔍
-5% - +4%
-1.49ms - +1.32ms
tip-of-tree
tip-of-tree
31.92ms - 33.66msunsure 🔍
-4% - +4%
-1.43ms - +1.33ms
-unsure 🔍
-4% - +3%
-1.38ms - +1.12ms
previous-release
previous-release
32.02ms - 33.83msunsure 🔍
-4% - +5%
-1.32ms - +1.49ms
unsure 🔍
-3% - +4%
-1.12ms - +1.38ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
80.74ms - 86.40ms-unsure 🔍
-6% - +3%
-4.90ms - +2.44ms
unsure 🔍
-9% - +1%
-8.35ms - +0.56ms
tip-of-tree
tip-of-tree
82.45ms - 87.14msunsure 🔍
-3% - +6%
-2.44ms - +4.90ms
-unsure 🔍
-8% - +2%
-6.83ms - +1.49ms
previous-release
previous-release
84.02ms - 90.90msunsure 🔍
-1% - +10%
-0.56ms - +8.35ms
unsure 🔍
-2% - +8%
-1.49ms - +6.83ms
-

nop-update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
22.80ms - 26.06ms-unsure 🔍
-14% - +3%
-3.76ms - +0.79ms
unsure 🔍
-15% - +1%
-3.94ms - +0.45ms
tip-of-tree
tip-of-tree
24.32ms - 27.50msunsure 🔍
-4% - +16%
-0.79ms - +3.76ms
-unsure 🔍
-9% - +7%
-2.43ms - +1.91ms
previous-release
previous-release
24.70ms - 27.65msunsure 🔍
-2% - +16%
-0.45ms - +3.94ms
unsure 🔍
-7% - +9%
-1.91ms - +2.43ms
-
lit-html-repeat

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
11.51ms - 12.32ms-unsure 🔍
-8% - +2%
-1.01ms - +0.23ms
unsure 🔍
-7% - +3%
-0.92ms - +0.41ms
tip-of-tree
tip-of-tree
11.84ms - 12.77msunsure 🔍
-2% - +9%
-0.23ms - +1.01ms
-unsure 🔍
-5% - +7%
-0.57ms - +0.84ms
previous-release
previous-release
11.64ms - 12.70msunsure 🔍
-3% - +8%
-0.41ms - +0.92ms
unsure 🔍
-7% - +5%
-0.84ms - +0.57ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
321.02ms - 358.38ms-unsure 🔍
-9% - +7%
-30.64ms - +25.10ms
unsure 🔍
-8% - +8%
-28.71ms - +26.02ms
tip-of-tree
tip-of-tree
321.79ms - 363.16msunsure 🔍
-7% - +9%
-25.10ms - +30.64ms
-unsure 🔍
-8% - +9%
-27.35ms - +30.21ms
previous-release
previous-release
321.04ms - 361.05msunsure 🔍
-8% - +8%
-26.02ms - +28.71ms
unsure 🔍
-9% - +8%
-30.21ms - +27.35ms
-
lit-html-template-heavy

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
54.94ms - 57.80ms-unsure 🔍
-2% - +5%
-1.01ms - +2.62ms
unsure 🔍
-1% - +5%
-0.81ms - +2.81ms
tip-of-tree
tip-of-tree
54.44ms - 56.69msunsure 🔍
-5% - +2%
-2.62ms - +1.01ms
-unsure 🔍
-3% - +3%
-1.39ms - +1.78ms
previous-release
previous-release
54.26ms - 56.48msunsure 🔍
-5% - +1%
-2.81ms - +0.81ms
unsure 🔍
-3% - +2%
-1.78ms - +1.39ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
123.16ms - 125.58ms-unsure 🔍
-2% - +1%
-3.12ms - +1.53ms
unsure 🔍
-1% - +2%
-0.69ms - +2.54ms
tip-of-tree
tip-of-tree
123.18ms - 127.15msunsure 🔍
-1% - +3%
-1.53ms - +3.12ms
-unsure 🔍
-0% - +3%
-0.54ms - +3.98ms
previous-release
previous-release
122.38ms - 124.51msunsure 🔍
-2% - +1%
-2.54ms - +0.69ms
unsure 🔍
-3% - +0%
-3.98ms - +0.54ms
-
reactive-element-list

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
52.86ms - 54.34ms-unsure 🔍
-2% - +2%
-1.02ms - +1.22ms
unsure 🔍
-1% - +3%
-0.38ms - +1.39ms
tip-of-tree
tip-of-tree
52.65ms - 54.34msunsure 🔍
-2% - +2%
-1.22ms - +1.02ms
-unsure 🔍
-1% - +3%
-0.57ms - +1.38ms
previous-release
previous-release
52.60ms - 53.58msunsure 🔍
-3% - +1%
-1.39ms - +0.38ms
unsure 🔍
-3% - +1%
-1.38ms - +0.57ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
871.84ms - 880.12ms-unsure 🔍
-0% - +1%
-2.27ms - +9.61ms
unsure 🔍
-0% - +1%
-0.35ms - +10.91ms
tip-of-tree
tip-of-tree
868.05ms - 876.57msunsure 🔍
-1% - +0%
-9.61ms - +2.27ms
-unsure 🔍
-0% - +1%
-4.11ms - +7.33ms
previous-release
previous-release
866.89ms - 874.51msunsure 🔍
-1% - +0%
-10.91ms - +0.35ms
unsure 🔍
-1% - +0%
-7.33ms - +4.11ms
-

update-reflect

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
883.87ms - 893.43ms-unsure 🔍
-0% - +1%
-2.16ms - +10.07ms
unsure 🔍
-0% - +1%
-4.31ms - +8.77ms
tip-of-tree
tip-of-tree
880.88ms - 888.51msunsure 🔍
-1% - +0%
-10.07ms - +2.16ms
-unsure 🔍
-1% - +0%
-7.60ms - +4.15ms
previous-release
previous-release
881.96ms - 890.88msunsure 🔍
-1% - +0%
-8.77ms - +4.31ms
unsure 🔍
-0% - +1%
-4.15ms - +7.60ms
-

tachometer-reporter-action v2 for Benchmarks

Copy link
Member

@augustjk augustjk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Much simpler this way.

@aomarks aomarks merged commit 52ab087 into main Feb 16, 2023
@aomarks aomarks deleted the localize-nested-html branch February 16, 2023 00:53
@lit-robot lit-robot mentioned this pull request Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[localize] Extracting html within html in messages
2 participants