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

[lit/context] Use target.set for setting context values #4598

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jun-sheaf
Copy link

@jun-sheaf jun-sheaf commented Mar 24, 2024

target.set is the correct way to set values in decorators. It supports nested decorators and private fields.

Copy link

changeset-bot bot commented Mar 24, 2024

🦋 Changeset detected

Latest commit: 8f1ced4

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

This PR includes changesets to release 1 package
Name Type
@lit/context 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 Mar 24, 2024

📊 Tachometer Benchmark Results

Summary

⏳ Benchmarks are currently running. Results below are out of date.

nop-update

  • this-change, tip-of-tree, previous-release: unsure 🔍 -5% - +4% (-0.53ms - +0.43ms)
    this-change vs tip-of-tree

render

  • this-change: 44.92ms - 46.61ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -5% - +2% (-0.88ms - +0.40ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -4% - +1% (-1.45ms - +0.45ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-0.49ms - +0.66ms)
    this-change vs tip-of-tree

update

  • this-change: 472.85ms - 482.49ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -8% - +4% (-3.21ms - +1.43ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-1.41ms - +1.36ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-4.59ms - +6.86ms)
    this-change vs tip-of-tree

update-reflect

  • this-change: 480.34ms - 487.90ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-5.38ms - +4.90ms)
    this-change vs tip-of-tree

Results

⏳ Benchmarks are currently running. Results below are out of date.
this-change

render

VersionAvg timevs
44.92ms - 46.61ms-

update

VersionAvg timevs
472.85ms - 482.49ms-

update-reflect

VersionAvg timevs
480.34ms - 487.90ms-
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.19ms - 19.01ms-unsure 🔍
-5% - +2%
-0.88ms - +0.40ms
unsure 🔍
-3% - +3%
-0.65ms - +0.52ms
tip-of-tree
tip-of-tree
18.35ms - 19.34msunsure 🔍
-2% - +5%
-0.40ms - +0.88ms
-unsure 🔍
-3% - +4%
-0.47ms - +0.83ms
previous-release
previous-release
18.25ms - 19.09msunsure 🔍
-3% - +4%
-0.52ms - +0.65ms
unsure 🔍
-4% - +2%
-0.83ms - +0.47ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
37.49ms - 40.64ms-unsure 🔍
-8% - +4%
-3.21ms - +1.43ms
unsure 🔍
-6% - +5%
-2.44ms - +2.01ms
tip-of-tree
tip-of-tree
38.26ms - 41.66msunsure 🔍
-4% - +8%
-1.43ms - +3.21ms
-unsure 🔍
-4% - +8%
-1.65ms - +2.99ms
previous-release
previous-release
37.71ms - 40.86msunsure 🔍
-5% - +6%
-2.01ms - +2.44ms
unsure 🔍
-7% - +4%
-2.99ms - +1.65ms
-

nop-update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
10.81ms - 11.50ms-unsure 🔍
-5% - +4%
-0.53ms - +0.43ms
unsure 🔍
-3% - +5%
-0.34ms - +0.60ms
tip-of-tree
tip-of-tree
10.87ms - 11.53msunsure 🔍
-4% - +5%
-0.43ms - +0.53ms
-unsure 🔍
-3% - +6%
-0.28ms - +0.64ms
previous-release
previous-release
10.70ms - 11.35msunsure 🔍
-5% - +3%
-0.60ms - +0.34ms
unsure 🔍
-6% - +3%
-0.64ms - +0.28ms
-
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.04ms - 34.02ms-unsure 🔍
-4% - +1%
-1.45ms - +0.45ms
unsure 🔍
-4% - +1%
-1.23ms - +0.41ms
tip-of-tree
tip-of-tree
33.21ms - 34.84msunsure 🔍
-1% - +4%
-0.45ms - +1.45ms
-unsure 🔍
-3% - +3%
-0.96ms - +1.14ms
previous-release
previous-release
33.28ms - 34.60msunsure 🔍
-1% - +4%
-0.41ms - +1.23ms
unsure 🔍
-3% - +3%
-1.14ms - +0.96ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
70.04ms - 72.16ms-unsure 🔍
-2% - +2%
-1.41ms - +1.36ms
unsure 🔍
-1% - +3%
-0.80ms - +2.09ms
tip-of-tree
tip-of-tree
70.23ms - 72.02msunsure 🔍
-2% - +2%
-1.36ms - +1.41ms
-unsure 🔍
-1% - +3%
-0.66ms - +2.00ms
previous-release
previous-release
69.47ms - 71.44msunsure 🔍
-3% - +1%
-2.09ms - +0.80ms
unsure 🔍
-3% - +1%
-2.00ms - +0.66ms
-
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
30.91ms - 31.72ms-unsure 🔍
-2% - +2%
-0.49ms - +0.66ms
unsure 🔍
-3% - +1%
-0.96ms - +0.41ms
tip-of-tree
tip-of-tree
30.82ms - 31.64msunsure 🔍
-2% - +2%
-0.66ms - +0.49ms
-unsure 🔍
-3% - +1%
-1.04ms - +0.32ms
previous-release
previous-release
31.04ms - 32.14msunsure 🔍
-1% - +3%
-0.41ms - +0.96ms
unsure 🔍
-1% - +3%
-0.32ms - +1.04ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
488.81ms - 497.39ms-unsure 🔍
-1% - +1%
-4.59ms - +6.86ms
unsure 🔍
-1% - +1%
-5.73ms - +5.80ms
tip-of-tree
tip-of-tree
488.18ms - 495.76msunsure 🔍
-1% - +1%
-6.86ms - +4.59ms
-unsure 🔍
-1% - +1%
-6.50ms - +4.30ms
previous-release
previous-release
489.22ms - 496.91msunsure 🔍
-1% - +1%
-5.80ms - +5.73ms
unsure 🔍
-1% - +1%
-4.30ms - +6.50ms
-

update-reflect

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
495.46ms - 503.55ms-unsure 🔍
-1% - +1%
-5.38ms - +4.90ms
unsure 🔍
-2% - +1%
-7.89ms - +3.35ms
tip-of-tree
tip-of-tree
496.57ms - 502.91msunsure 🔍
-1% - +1%
-4.90ms - +5.38ms
-unsure 🔍
-1% - +1%
-7.05ms - +2.99ms
previous-release
previous-release
497.88ms - 505.67msunsure 🔍
-1% - +2%
-3.35ms - +7.89ms
unsure 🔍
-1% - +1%
-2.99ms - +7.05ms
-

tachometer-reporter-action v2 for Benchmarks

@justinfagnani justinfagnani self-requested a review March 24, 2024 15:18
Copy link
Contributor

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

@jun-sheaf jun-sheaf changed the title [lit/context] Use access.set for setting context values [lit/context] Use target.set for setting context values Mar 24, 2024
@justinfagnani
Copy link
Collaborator

Thanks @jun-sheaf!

We need a changeset file as part of the PR. See https://github.com/lit/lit/blob/main/CONTRIBUTING.md#pull-requests

@jun-sheaf jun-sheaf force-pushed the jrandolf/use-access.set branch 2 times, most recently from 8b18d66 to 25a2aae Compare March 26, 2024 18:13
@jun-sheaf
Copy link
Author

Thanks @jun-sheaf!

We need a changeset file as part of the PR. See https://github.com/lit/lit/blob/main/CONTRIBUTING.md#pull-requests

Done.

`target.set` is the correct way to set values in decorators. It supports nested decorators and private fields.
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.

None yet

2 participants