-
Notifications
You must be signed in to change notification settings - Fork 438
feat: signals implementation v1 #3963
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
Changes from 17 commits
4a0e5f7
f7f658d
0a08b45
66f885b
65fa1cb
8037a6c
7b3582f
92413fc
fcdae16
cf3bafd
b06528b
f315852
4ec0439
3c78f17
e3b6e82
2fa1693
1565a04
6fde22a
3c6aa00
1565db7
6360bb6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -4,13 +4,16 @@ | |||
| * SPDX-License-Identifier: MIT | ||||
| * For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT | ||||
| */ | ||||
| import { isFunction, isNull, isObject } from '@lwc/shared'; | ||||
| import { Signal } from '@lwc/signals'; | ||||
| import { | ||||
| JobFunction, | ||||
| CallbackFunction, | ||||
| ReactiveObserver, | ||||
| valueMutated, | ||||
| valueObserved, | ||||
| } from '../libs/mutation-tracker'; | ||||
| import { subscribeToSignal } from '../libs/signal-tracker'; | ||||
| import { VM } from './vm'; | ||||
|
|
||||
| const DUMMY_REACTIVE_OBSERVER = { | ||||
|
|
@@ -28,10 +31,29 @@ export function componentValueMutated(vm: VM, key: PropertyKey) { | |||
| } | ||||
| } | ||||
|
|
||||
| export function componentValueObserved(vm: VM, key: PropertyKey) { | ||||
| export function componentValueObserved(vm: VM, key: PropertyKey, target: any = {}) { | ||||
| const { component, tro } = vm; | ||||
| // On the server side, we don't need mutation tracking. Skipping it improves performance. | ||||
| if (process.env.IS_BROWSER) { | ||||
| valueObserved(vm.component, key); | ||||
| valueObserved(component, key); | ||||
| } | ||||
|
|
||||
| // The portion of reactivity that's exposed to signals is to subscribe a callback to re-render the VM (templates). | ||||
| // We check check the following to ensure re-render is subscribed at the correct time. | ||||
| // 1. The template is currently being rendered (there is a template reactive observer) | ||||
| // 2. There was a call to a getter to access the signal (happens during vnode generation) | ||||
| if ( | ||||
| lwcRuntimeFlags.ENABLE_EXPERIMENTAL_SIGNALS && | ||||
| isObject(target) && | ||||
| !isNull(target) && | ||||
| 'value' in target && | ||||
| 'subscribe' in target && | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
No need since you're already checking it's a function below.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nolanlawson typescript is complaining when I do |
||||
| isFunction(target.subscribe) && | ||||
| // Only subscribe if a template is being rendered by the engine | ||||
| tro.isObserving() | ||||
| ) { | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These conditions look right to me for now. Sometime after this PR lands, we may want to replace the last three conditions with something like |
||||
| // Subscribe the template reactive observer's notify method, which will mark the vm as dirty and schedule hydration. | ||||
| subscribeToSignal(component, target as Signal<any>, tro.notify.bind(tro)); | ||||
| } | ||||
| } | ||||
|
|
||||
|
|
@@ -41,3 +63,4 @@ export function createReactiveObserver(callback: CallbackFunction): ReactiveObse | |||
| } | ||||
|
|
||||
| export * from '../libs/mutation-tracker'; | ||||
| export * from '../libs/signal-tracker'; | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| /* | ||
| * Copyright (c) 2024, salesforce.com, inc. | ||
| * All rights reserved. | ||
| * SPDX-License-Identifier: MIT | ||
| * For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT | ||
| */ | ||
| import { isFalse, isFunction, isUndefined } from '@lwc/shared'; | ||
| import { Signal } from '@lwc/signals'; | ||
| import { logWarnOnce } from '../../shared/logger'; | ||
|
|
||
| /** | ||
| * This map keeps track of objects to signals. There is an assumption that the signal is strongly referenced | ||
| * on the object which allows the SignalTracker to be garbage collected along with the object. | ||
| */ | ||
| const TargetToSignalTrackerMap = new WeakMap<object, SignalTracker>(); | ||
|
|
||
| function getSignalTracker(target: object) { | ||
| let signalTracker = TargetToSignalTrackerMap.get(target); | ||
| if (isUndefined(signalTracker)) { | ||
| signalTracker = new SignalTracker(); | ||
| TargetToSignalTrackerMap.set(target, signalTracker); | ||
| } | ||
| return signalTracker; | ||
| } | ||
|
|
||
| export function subscribeToSignal( | ||
| target: Object, | ||
| signal: Signal<unknown>, | ||
| update: CallbackFunction | ||
| ) { | ||
| const signalTracker = getSignalTracker(target); | ||
| if (isFalse(signalTracker.seen(signal))) { | ||
| signalTracker.subscribeToSignal(signal, update); | ||
| } | ||
| } | ||
|
|
||
| export function unsubscribeFromSignals(target: object) { | ||
| if (TargetToSignalTrackerMap.has(target)) { | ||
| const signalTracker = getSignalTracker(target); | ||
| signalTracker.unsubscribeFromSignals(); | ||
| signalTracker.reset(); | ||
| } | ||
| } | ||
|
|
||
| type CallbackFunction = () => void; | ||
|
|
||
| /** | ||
| * This class is used to keep track of the signals associated to a given object. | ||
| * It is used to prevent the LWC engine from subscribing duplicate callbacks multiple times | ||
| * to the same signal. Additionally, it keeps track of all signal unsubscribe callbacks, handles invoking | ||
| * them when necessary and discarding them. | ||
| */ | ||
| class SignalTracker { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created this module to keep track of LWC component instances to signals bound the LWC (the signal is attached to the LWC class somewhere). The reason this is needed is because during the rendering process, if a signal is referenced more than once on a template its getter is called multiple times. Since we're hooking into the class' getter to determine whether a property access is a signal, there may be multiple attempts to subscribe the same callback to a single signal. I thought about reusing the The |
||
| private signalToUnsubscribeMap: Map<Signal<unknown>, CallbackFunction> = new Map(); | ||
|
|
||
| seen(signal: Signal<unknown>) { | ||
| return this.signalToUnsubscribeMap.has(signal); | ||
| } | ||
|
|
||
| subscribeToSignal(signal: Signal<unknown>, update: CallbackFunction) { | ||
| try { | ||
| const unsubscribe = signal.subscribe(update); | ||
| if (isFunction(unsubscribe)) { | ||
| // TODO [#3978]: Evaluate how we should handle the case when unsubscribe is not a function. | ||
| // Long term we should throw an error or log a warning. | ||
| this.signalToUnsubscribeMap.set(signal, unsubscribe); | ||
| } | ||
| } catch (err: any) { | ||
| logWarnOnce( | ||
| `Attempted to subscribe to an object that has the shape of a signal but received the following error: ${ | ||
| err?.stack ?? err | ||
| }` | ||
| ); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably have a call to action here, i.e. tell the user what they need to do to resolve the error. AIUI, this will occur if e.g. a plain object has a shape like If that's the case, then I don't know if we even want to warn here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're checking We definitely don't want errors in
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nolanlawson the signal shape should be I'll have to do some more research on how other frameworks handle this situation but I know most of them have their own signal implementations, so they may not run into this issue. I'll log the stack trace for now as @divmain suggested and look into how other frameworks handle this in more detail. |
||
| } | ||
| } | ||
|
|
||
| unsubscribeFromSignals() { | ||
| try { | ||
| this.signalToUnsubscribeMap.forEach((unsubscribe) => unsubscribe()); | ||
| } catch (err: any) { | ||
| logWarnOnce( | ||
| `Attempted to call a signal's unsubscribe callback but received the following error: ${ | ||
| err?.stack ?? err | ||
| }` | ||
| ); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one seems a bit more serious to me. In this case, we might have landed here because a signal exposed a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Elsewhere, we are checking that Same ask as earlier for preserving the error's stack.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nolanlawson In For v1 I'll log the stack trace as @divmain suggested but I left a todo (#3978) to revisit this in 252 once we figure out the use cases and know how we want to better handle the errors. |
||
| } | ||
| } | ||
|
|
||
| reset() { | ||
| this.signalToUnsubscribeMap.clear(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,7 @@ This set of environment variables applies to the `start` and `test` commands: | |
| - **`API_VERSION=<version>`:** API version to use when compiling. | ||
| - **`DISABLE_SYNTHETIC_SHADOW_SUPPORT_IN_COMPILER=1`:** Disable synthetic shadow in the compiler itself. | ||
| - **`DISABLE_STATIC_CONTENT_OPTIMIZATION=1`:** Disable static content optimization by setting `enableStaticContentOptimization` to `false`. | ||
| - **`ENABLE_EXPERIMENTAL_SIGNALS=1`:** Enables tests for experimental signals protocol. | ||
|
||
|
|
||
| ## Examples | ||
|
|
||
|
|
||
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.
This should be a dependency, or else it's gonna break things! (Alternatively, make the imports in
mutation-tracker/signal-trackerconditional.)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.
@wjhsf The way
devDependencieswork in this repo is that they get inlined into the builtdistfiles. So we use this as a kind of hack when we want certain built files not to contain anyrequires/imports.In this case, I think this is the right approach since historically
@lwc/engine-coredoesn't have any imports. The question then is why not just make@lwc/signalspart of@lwc/engine-coreitself.The answer to that, I think, is that
@lwc/signalskind of acts as a documentation hub and an ergonomic way to import a base class that consumers can extend. The value of this does seem debatable to me (we could just export the same thing from@lwc/engine-dom/lwc), and historically we've annoyed users with a proliferation of@lwc/*repos they've had to install, and plus this is an experimental feature, so I would lean towards just moving it back to@lwc/engine-corefor now.