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-tools] Preserve existing placeholder id/index on extract #4503

Merged
merged 4 commits into from
Jan 18, 2024

Conversation

augustjk
Copy link
Contributor

@augustjk augustjk commented Jan 18, 2024

Fixes #4490

Instead of creating a new placeholder index on write which will always be in the order seen, we should use the existing index for untranslatables which could be out of order if they're read from an already translated target.

Copy link

changeset-bot bot commented Jan 18, 2024

🦋 Changeset detected

Latest commit: 478e693

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

Copy link
Contributor

github-actions bot commented Jan 18, 2024

📊 Tachometer Benchmark Results

Summary

nop-update

  • this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +6% (-0.32ms - +0.72ms)
    this-change vs tip-of-tree

render

  • this-change: 49.46ms - 52.19ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -4% - +3% (-0.72ms - +0.64ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -4% - +2% (-1.24ms - +0.53ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -0% - +3% (-0.14ms - +1.11ms)
    this-change vs tip-of-tree

update

  • this-change: 532.68ms - 543.62ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -6% - +8% (-2.32ms - +3.33ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +2% (-1.88ms - +1.84ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-4.61ms - +3.83ms)
    this-change vs tip-of-tree

update-reflect

  • this-change: 536.12ms - 549.61ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -0% - +1% (-2.71ms - +5.34ms)
    this-change vs tip-of-tree

Results

this-change

render

VersionAvg timevs
49.46ms - 52.19ms-

update

VersionAvg timevs
532.68ms - 543.62ms-

update-reflect

VersionAvg timevs
536.12ms - 549.61ms-
this-change, tip-of-tree, previous-release

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
18.24ms - 19.17ms-unsure 🔍
-4% - +3%
-0.72ms - +0.64ms
unsure 🔍
-4% - +3%
-0.82ms - +0.62ms
tip-of-tree
tip-of-tree
18.25ms - 19.24msunsure 🔍
-3% - +4%
-0.64ms - +0.72ms
-unsure 🔍
-4% - +4%
-0.80ms - +0.68ms
previous-release
previous-release
18.26ms - 19.36msunsure 🔍
-3% - +4%
-0.62ms - +0.82ms
unsure 🔍
-4% - +4%
-0.68ms - +0.80ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
40.15ms - 44.19ms-unsure 🔍
-6% - +8%
-2.32ms - +3.33ms
unsure 🔍
-5% - +8%
-2.05ms - +3.22ms
tip-of-tree
tip-of-tree
39.69ms - 43.64msunsure 🔍
-8% - +5%
-3.33ms - +2.32ms
-unsure 🔍
-6% - +6%
-2.52ms - +2.68ms
previous-release
previous-release
39.89ms - 43.28msunsure 🔍
-8% - +5%
-3.22ms - +2.05ms
unsure 🔍
-6% - +6%
-2.68ms - +2.52ms
-

nop-update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
11.25ms - 11.95ms-unsure 🔍
-3% - +6%
-0.32ms - +0.72ms
unsure 🔍
-5% - +3%
-0.59ms - +0.35ms
tip-of-tree
tip-of-tree
11.02ms - 11.78msunsure 🔍
-6% - +3%
-0.72ms - +0.32ms
-unsure 🔍
-7% - +2%
-0.81ms - +0.18ms
previous-release
previous-release
11.40ms - 12.03msunsure 🔍
-3% - +5%
-0.35ms - +0.59ms
unsure 🔍
-2% - +7%
-0.18ms - +0.81ms
-
this-change, tip-of-tree, previous-release

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
33.75ms - 34.75ms-unsure 🔍
-4% - +2%
-1.24ms - +0.53ms
unsure 🔍
-0% - +3%
-0.15ms - +1.15ms
tip-of-tree
tip-of-tree
33.88ms - 35.33msunsure 🔍
-2% - +4%
-0.53ms - +1.24ms
-slower ❌
0% - 5%
0.02ms - 1.69ms
previous-release
previous-release
33.34ms - 34.16msunsure 🔍
-3% - +0%
-1.15ms - +0.15ms
faster ✔
0% - 5%
0.02ms - 1.69ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
72.44ms - 74.86ms-unsure 🔍
-3% - +2%
-1.88ms - +1.84ms
unsure 🔍
-1% - +3%
-0.61ms - +2.52ms
tip-of-tree
tip-of-tree
72.26ms - 75.09msunsure 🔍
-2% - +3%
-1.84ms - +1.88ms
-unsure 🔍
-1% - +4%
-0.76ms - +2.71ms
previous-release
previous-release
71.70ms - 73.69msunsure 🔍
-3% - +1%
-2.52ms - +0.61ms
unsure 🔍
-4% - +1%
-2.71ms - +0.76ms
-
this-change, tip-of-tree, previous-release

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
34.54ms - 35.48ms-unsure 🔍
-0% - +3%
-0.14ms - +1.11ms
unsure 🔍
-1% - +3%
-0.21ms - +1.01ms
tip-of-tree
tip-of-tree
34.10ms - 34.94msunsure 🔍
-3% - +0%
-1.11ms - +0.14ms
-unsure 🔍
-2% - +1%
-0.66ms - +0.49ms
previous-release
previous-release
34.22ms - 35.00msunsure 🔍
-3% - +1%
-1.01ms - +0.21ms
unsure 🔍
-1% - +2%
-0.49ms - +0.66ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
548.78ms - 554.98ms-unsure 🔍
-1% - +1%
-4.61ms - +3.83ms
unsure 🔍
-0% - +1%
-1.99ms - +6.81ms
tip-of-tree
tip-of-tree
549.40ms - 555.13msunsure 🔍
-1% - +1%
-3.83ms - +4.61ms
-unsure 🔍
-0% - +1%
-1.44ms - +7.04ms
previous-release
previous-release
546.34ms - 552.59msunsure 🔍
-1% - +0%
-6.81ms - +1.99ms
unsure 🔍
-1% - +0%
-7.04ms - +1.44ms
-

update-reflect

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
557.67ms - 563.44ms-unsure 🔍
-0% - +1%
-2.71ms - +5.34ms
slower ❌
0% - 1%
0.12ms - 7.62ms
tip-of-tree
tip-of-tree
556.44ms - 562.05msunsure 🔍
-1% - +0%
-5.34ms - +2.71ms
-unsure 🔍
-0% - +1%
-1.13ms - +6.25ms
previous-release
previous-release
554.29ms - 559.08msfaster ✔
0% - 1%
0.12ms - 7.62ms
unsure 🔍
-1% - +0%
-6.25ms - +1.13ms
-

tachometer-reporter-action v2 for Benchmarks

Copy link
Contributor

The size of lit-html.js and lit-core.min.js are as expected.

Copy link
Contributor

@AndrewJakubowicz AndrewJakubowicz left a comment

Choose a reason for hiding this comment

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

Thank you!

@augustjk augustjk merged commit 350147d into main Jan 18, 2024
9 checks passed
@augustjk augustjk deleted the localize-extract-ph-order branch January 18, 2024 23:34
@lit-robot lit-robot mentioned this pull request Jan 31, 2024
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] extract command breaks translation targets with reordered expression placeholders
2 participants