WIP: Add @lit-labs/signals package #4615
Conversation
|
📊 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.
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.
| @@ -0,0 +1,127 @@ | |||
| # @lit-labs/preact-signals | |||
|
|
|||
| Preact Signals integration for Lit. | |||
There was a problem hiding this comment.
+ the "why preact signals" section. Perhaps even worth updating the preact-signals labs package readme to point to this one
|
|
||
| render() { | ||
| return html` | ||
| <p>The count is ${count.value}</p> |
| } | ||
|
|
||
| private _onClick() { | ||
| count.value = count.value + 1; |
| } | ||
|
|
||
| private _onClick() { | ||
| count.value = count.value + 1; |
| } | ||
|
|
||
| private _onClick() { | ||
| count.value = count.value + 1; |
|
|
||
| override connectedCallback(): void { | ||
| if (!this.__isWatching) { | ||
| this.__isWatching = true; |
There was a problem hiding this comment.
Why is the flag needed? when would this happen when __isWatching is true? And I believe you can use Signal.subtle.hasSources(this.__watcher) if the state is needed (although an explicit flag in that case might be simpler).
| private __forceUpdateSignal = new Signal.State(0); | ||
| private __updateSignal = new Signal.Computed(() => { | ||
| this.__forceUpdateSignal.get(); | ||
| super.performUpdate(); |
There was a problem hiding this comment.
I think you have to call this.__watcher.watch() here to make sure to re-activate the watcher. Make sure to avoid passing the signal due to tc39/proposal-signals#157
| ): void { | ||
| this.__shouldRender = true; | ||
| if (!this.__watcherUpdate) { | ||
| this.__forceUpdateSignal?.set(this.__forceUpdateSignal.get() + 1); |
There was a problem hiding this comment.
Is the idea that this ensures the update occurs even if performUpdate didn't get any signals? (e.g. you didn't actually use any signals)? If so, let's comment that.
| this.__shouldRender = false; | ||
| super.update(changedProperties); | ||
| } else { | ||
| this.__pendingWatches.forEach((d) => d.commmit()); |
There was a problem hiding this comment.
Calling commit here couples this pretty tightly with the directive. This is why I just passed a callback. No strong opinion though.
| }); | ||
|
|
||
| commmit() { | ||
| this.setValue(Signal.subtle.untrack(() => this.__signal?.get())); |
There was a problem hiding this comment.
Again here, I think you have to call this.__watcher.watch() to reset the watcher.
| @@ -0,0 +1,127 @@ | |||
| # @lit-labs/preact-signals | |||
|
|
|||
| Preact Signals integration for Lit. | |||
There was a problem hiding this comment.
+ the "why preact signals" section. Perhaps even worth updating the preact-signals labs package readme to point to this one
| export const defaultConfig = (options = {}) => | ||
| litProdConfig({ | ||
| packageName: createRequire(import.meta.url)('./package.json').name, | ||
| entryPoints: ['index', 'lib/signal-watcher', 'lib/watch'], |
There was a problem hiding this comment.
Why do we separate watch and signal-watcher but not the html tag module?
| @@ -0,0 +1,17 @@ | |||
| /** | |||
| * @license | |||
| * Copyright 2018 Google LLC | |||
There was a problem hiding this comment.
Consider updating copyright years
| // one is available. See https://github.com/preactjs/signals/issues/402 | ||
| return coreTag( | ||
| strings, | ||
| ...values.map((v) => (v instanceof Signal.State || v instanceof Signal.Computed? watch(v) : v)) |
There was a problem hiding this comment.
How would we recommend handling ternaries that use a signal here or a signal.get().map(...)? I think this answer is different for SignalWatcher() and non-SignalWatcher() elements. SignalWatcher() elements using this html tag for convenience should just .get() the value and it works:
class MyElement extends SignalWatcher(LitElement) {
...
html`Clicked ${count} time${count.get() !== 1 ? 's' : nothing}.`;
...For non-SignalWatcher() elements we need something like computed or for watch() to handle watchable functions natively:
class MyElement extends LitElement {
...
html`Clicked ${count} time${computed(() => (count.get() !== 1 ? 's' : nothing))}.`;
// some sort of function that is marked as watchable because we do not
// want to arbitrarily call bound functions like
// html`<my-textfield .checkValidity=${(value) => {...}}>`
html`Clicked ${count} time${() => (count.get() !== 1 ? 's' : nothing)}.`;
...Also i think mentioned elsewhere in the PR but would be cool to opt-out of watch() in some cases to pass the signal reference directly.
| Base: T | ||
| ): T { | ||
| abstract class SignalWatcher extends Base { | ||
| // Watcher.watch() doesn't dedupe so we need to track this ourselves. |
There was a problem hiding this comment.
Link to bug in comment
| export const Computed = Signal.Computed; | ||
| export const subtle = Signal.subtle; | ||
|
|
||
| export const signal = <T>(value: T) => new Signal.State(value); |
There was a problem hiding this comment.
optional options arg for equality fn is a must
|
|
||
| override disconnectedCallback(): void { | ||
| this.__isWatching = false; | ||
| this.__watcher.unwatch(this.__updateSignal); |
There was a problem hiding this comment.
what do we think about allowing users to unwatch the component? For example, in an MPA people might not want a component that's connected but not visible to re-render due to signal changes. Probably punt this until it's a real problem
| // @ts-expect-error: signal is unused, but the name appears in the signature | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| render(signal: Signal.State<T> | Signal.Computed<T>): T { | ||
| return undefined as T; |
There was a problem hiding this comment.
SSR sad, is there really no good solution here with the current proposal, even with an unwatch?
| if (this.__signal !== undefined) { | ||
| this.__watcher.unwatch(this.__signal); | ||
| } | ||
| if (signal !== undefined) { |
There was a problem hiding this comment.
When is this false? If it's ever false won't line 49 throw?
|
@justinfagnani any progress? |
WIP: Still some rough edges with trying to sync watch() updates and regular updates.