-
Notifications
You must be signed in to change notification settings - Fork 923
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
Add @lit-labs/signals package #4615
Conversation
🦋 Changeset detectedLatest commit: 52dd697 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For watch, let's consider @e111077 's feedback and maybe have a way to auto-wrap a function (maybe if it's not an event part?).
And I really like using the html
and not manually watching but if we do that we probably want unwatched
which is just basically an echoing directive.
@justinfagnani any progress? |
afacc80
to
e3528f6
Compare
@JosefJezek just pushed a major update. Hopefully we land this soon. |
When this is merged we'll need to create a Lit Labs page entry on lit.dev and a feedback discussion thread similar to #4115 |
Co-authored-by: Pascal Schilp <pascalschilp@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The memory approach is clever. Wonder if we might run into issues down the line with watchers running unnecessarily because of FinalizationRegistry timing. Though only things I could think of that might run into that are users writing brittle code that is probably in need of a rewrite anyway
All the current test failures are from virtualizer. Re-running tests hasn't made them pass, so I'm not sure if they're flaky. |
I tried to update test dependencies in #4758 to see if that would help the virtualizer tests. That didn't work locally at least, so I'll keep re-running the test job for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL!
@graynorton I had to disable the virtualizer tests here because they wouldn't pass even on multiple test re-runs. This PR doesn't change any core or virtualizer code, nor does it update any relevant dependencies int he package lock. I'm not sure how to move forward there. Any advice would be helpful! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All my comments at this point are just doc nits. Code looks good to me
Co-authored-by: Elliott Marquez <5981958+e111077@users.noreply.github.com>
WIP: Still some rough edges with trying to sync watch() updates and regular updates.