Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/@lwc/engine-core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
"@lwc/shared": "6.0.0"
},
"devDependencies": {
"observable-membrane": "2.0.0"
"observable-membrane": "2.0.0",
"@lwc/signals": "6.0.0"
Copy link
Contributor

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-tracker conditional.)

Copy link
Contributor

Choose a reason for hiding this comment

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

@wjhsf The way devDependencies work in this repo is that they get inlined into the built dist files. So we use this as a kind of hack when we want certain built files not to contain any requires/imports.

In this case, I think this is the right approach since historically @lwc/engine-core doesn't have any imports. The question then is why not just make @lwc/signals part of @lwc/engine-core itself.

The answer to that, I think, is that @lwc/signals kind 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-core for now.

}
}
9 changes: 8 additions & 1 deletion packages/@lwc/engine-core/src/framework/component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ import {
LOWEST_API_VERSION,
} from '@lwc/shared';

import { createReactiveObserver, ReactiveObserver } from './mutation-tracker';
import {
createReactiveObserver,
ReactiveObserver,
unsubscribeFromSignals,
} from './mutation-tracker';

import { invokeComponentRenderMethod, isInvokingRender, invokeEventListener } from './invoker';
import { VM, scheduleRehydration } from './vm';
Expand Down Expand Up @@ -92,6 +96,9 @@ export function renderComponent(vm: VM): VNodes {
}

vm.tro.reset();
if (lwcRuntimeFlags.ENABLE_EXPERIMENTAL_SIGNALS) {
unsubscribeFromSignals(vm.component);
}
const vnodes = invokeComponentRenderMethod(vm);
vm.isDirty = false;
vm.isScheduled = false;
Expand Down
5 changes: 3 additions & 2 deletions packages/@lwc/engine-core/src/framework/decorators/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,9 @@ export function createPublicPropertyDescriptor(key: string): PropertyDescriptor
}
return;
}
componentValueObserved(vm, key);
return vm.cmpProps[key];
const val = vm.cmpProps[key];
componentValueObserved(vm, key, val);
return val;
},
set(this: LightningElement, newValue: any) {
const vm = getAssociatedVM(this);
Expand Down
5 changes: 3 additions & 2 deletions packages/@lwc/engine-core/src/framework/decorators/track.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,9 @@ export function internalTrackDecorator(key: string): PropertyDescriptor {
return {
get(this: LightningElement): any {
const vm = getAssociatedVM(this);
componentValueObserved(vm, key);
return vm.cmpFields[key];
const val = vm.cmpFields[key];
componentValueObserved(vm, key, val);
return val;
},
set(this: LightningElement, newValue: any) {
const vm = getAssociatedVM(this);
Expand Down
25 changes: 23 additions & 2 deletions packages/@lwc/engine-core/src/framework/mutation-tracker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
valueMutated,
valueObserved,
} from '../libs/mutation-tracker';
import { subscribeToSignal } from '../libs/signal-tracker';
import { VM } from './vm';

const DUMMY_REACTIVE_OBSERVER = {
Expand All @@ -28,10 +29,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 &&
target &&
typeof target === 'object' &&
'value' in target &&
'subscribe' in target &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'subscribe' in target &&

No need since you're already checking it's a function below.

Copy link
Member Author

@jmsjtu jmsjtu Feb 6, 2024

Choose a reason for hiding this comment

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

@nolanlawson typescript is complaining when I do isFunction(target.subscribe), without checking for 'subscribe' in target so I added it back for now.

typeof target.subscribe === 'function' &&
// Only subscribe if a template is being rendered by the engine
tro.isObserving()
) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 && target.isSignal === SUPER_SECRET_SYMBOL, where const SUPER_SECRET_SYMBOL === Symbol("SymbolSigil"). This would allow us to lock down initial usage of the signal machinery by restricting who has access to that symbol in the runtime (i.e. so that customers can't start using signals before we're ready).

// Subscribe the template reactive observer's notify method, which will mark the vm as dirty and schedule hydration.
subscribeToSignal(component, target, tro.notify.bind(tro));
Copy link
Member Author

@jmsjtu jmsjtu Feb 2, 2024

Choose a reason for hiding this comment

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

You might be wondering why I'm exporting and using the signal-tracker inside the mutation-tracker in @lwc/engine-core when I was trying to keep the mutation-tracker library clean.

Note mutation-tracker in @lwc/engine-core is a separate package from mutation-tracker in libs

The reason is because the only real function I could see exporting from a signal-tracker module in @lwc/engine-core was something that would export a similar function as valueObserved.

Rather than create a separate function, I decided to reuse it since it's already aware of things outside the mutation-tracker library, like the vm.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
subscribeToSignal(component, target, tro.notify.bind(tro));
subscribeToSignal(component, target, FunctionBind.call(tro.notify, tro));

Copy link
Member Author

Choose a reason for hiding this comment

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

@nolanlawson it looks like we don't have a FunctionBind extracted in @lwc/shared.

I plan to open a separate PR to expose some of the common function methods and I'll update it here as part of the PR.

}
}

Expand All @@ -41,3 +61,4 @@ export function createReactiveObserver(callback: CallbackFunction): ReactiveObse
}

export * from '../libs/mutation-tracker';
export * from '../libs/signal-tracker';
5 changes: 3 additions & 2 deletions packages/@lwc/engine-core/src/framework/observed-fields.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@ export function createObservedFieldPropertyDescriptor(key: string): PropertyDesc
return {
get(this: LightningElement): any {
const vm = getAssociatedVM(this);
componentValueObserved(vm, key);
return vm.cmpFields[key];
const val = vm.cmpFields[key];
componentValueObserved(vm, key, val);
return val;
},
set(this: LightningElement, newValue: any) {
const vm = getAssociatedVM(this);
Expand Down
7 changes: 5 additions & 2 deletions packages/@lwc/engine-core/src/framework/vm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ import {
logGlobalOperationStart,
} from './profiler';
import { patchChildren } from './rendering';
import { ReactiveObserver } from './mutation-tracker';
import { ReactiveObserver, unsubscribeFromSignals } from './mutation-tracker';
import { connectWireAdapters, disconnectWireAdapters, installWireAdapters } from './wiring';
import {
VNodes,
Expand Down Expand Up @@ -272,9 +272,12 @@ function resetComponentStateWhenRemoved(vm: VM) {
const { state } = vm;

if (state !== VMState.disconnected) {
const { tro } = vm;
const { tro, component } = vm;
// Making sure that any observing record will not trigger the rehydrated on this vm
tro.reset();
if (lwcRuntimeFlags.ENABLE_EXPERIMENTAL_SIGNALS) {
unsubscribeFromSignals(component);
}
runDisconnectedCallback(vm);
// Spec: https://dom.spec.whatwg.org/#concept-node-remove (step 14-15)
runChildNodesDisconnectedCallback(vm);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,4 +125,8 @@ export class ReactiveObserver {
// we keep track of observing records where the observing record was added to so we can do some clean up later on
ArrayPush.call(this.listeners, reactiveObservers);
}

isObserving() {
return currentReactiveObserver === this;
}
}
84 changes: 84 additions & 0 deletions packages/@lwc/engine-core/src/libs/signal-tracker/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* 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, 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: WeakMap<Object, SignalTracker> = new WeakMap();

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 {
Copy link
Member Author

Choose a reason for hiding this comment

The 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 mutation-tracker library to track this information but decided against it to keep the abstraction clean.

The mutation-tracker library only really cares about the state within its own module and I felt introducing the concept of signals to it would pollute the abstraction.

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);
this.signalToUnsubscribeMap.set(signal, unsubscribe);
} catch (err) {
logWarnOnce(
`Attempted to subscribe to an object that has the shape of a signal but received the following error: ${err}`
);
Copy link
Contributor

Choose a reason for hiding this comment

The 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 { value: 'foo' }, right? Meaning we mistakenly sniff it as a signal?

If that's the case, then I don't know if we even want to warn here. value is a pretty common name, and I imagine users may be annoyed by even one warning message, if there's nothing they can do about it. What do other signal-using frameworks do in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

We're checking typeof obj.subscribe === 'function' elsewhere, so that is not the case that'd be hit here. Instead, this would occur if an error threw while inside the subscribe() call.

We definitely don't want errors in subscribe() to be swallowed entirely. However, I think coercing into a string here might not be the right thing. For example, you'd lose the call stack. On the other hand, a thrown object isn't guaranteed to be an actual error object. So maybe something like this:

logWarnOnce(`Attempted to subscribe to...: ${err?.stack ?? err}`);

Copy link
Member Author

Choose a reason for hiding this comment

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

@nolanlawson the signal shape should be { value: 'foo' , subscribe: () => {} }.

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) {
logWarnOnce(
`Attempted to call a signal's unsubscribe callback but received the following error: ${err}`
);
Copy link
Contributor

Choose a reason for hiding this comment

The 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 value and a subscribe, but not an unsubscribe. (I.e. you may have a signal implementation that is prone to memory leaks, since it doesn't implement unsubscribe.) I wonder if there is value in separating out the two cases and providing a better error message for each scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

Elsewhere, we are checking that typeof unsubscribe === 'function'. So we know at this point that unsubscribe is invokable. However, similar to the other condition, this could occur if an error occurs during unsubscribe, e.g. the signal wraps a websocket connection, unsubscribe() attempts to ws.close() and that fails for some reason.

Same ask as earlier for preserving the error's stack.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nolanlawson In SignalTracker.subscribeToSignal we now check that the subscribe returns an unsubscribe function and only stores if it's a function.

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();
}
}
1 change: 1 addition & 0 deletions packages/@lwc/features/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const features: FeatureFlagMap = {
ENABLE_FROZEN_TEMPLATE: null,
ENABLE_LEGACY_SCOPE_TOKENS: null,
ENABLE_FORCE_SHADOW_MIGRATE_MODE: null,
ENABLE_EXPERIMENTAL_SIGNALS: null,
};

// eslint-disable-next-line no-restricted-properties
Expand Down
6 changes: 6 additions & 0 deletions packages/@lwc/features/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ export interface FeatureFlagMap {
* If true, enable experimental shadow DOM migration mode globally.
*/
ENABLE_FORCE_SHADOW_MIGRATE_MODE: FeatureFlagValue;

/**
* EXPERIMENTAL FEATURE, DO NOT USE IN PRODUCTION
* If true, allows the engine to expose reactivity to signals as describe in @lwc/signals.
*/
ENABLE_EXPERIMENTAL_SIGNALS: FeatureFlagValue;
}

export type FeatureFlagName = keyof FeatureFlagMap;
75 changes: 75 additions & 0 deletions packages/@lwc/signals/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
# @lwc/signals

This is an experimental package containing the interface expected for signals.

A key point to note is that when a signal is both bound to an LWC class member variable and used on a template,
the LWC engine will attempt to subscribe a callback to rerender the template.
Comment on lines +3 to +6
Copy link
Member Author

Choose a reason for hiding this comment

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

I wrote a quick readme based on our internal docs and the RFC that caridy wrote. Open to changing the wording on any of this!

Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned above, I think for now this should probably just be in @lwc/engine-core. We can decide later if we want to expose a brand-new package out of this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nolanlawson let's chat offline about this. Having a separate package will help facilitate the related work that is coevolving.


## Reactivity with Signals

A Signal is an object that holds a value and allows components to react to changes to that value.
It exposes a `.value` property for accessing the current value, and `.subscribe` methods for responding to changes.

```js
import { signal } from 'some/signals';

export default class ExampleComponent extends LightningElement {
count = signal(0);

increment() {
this.count.value++;
}
}
```

In the template, we can bind directly to the `.value` property:

```html
<template>
<button onclick="{increment}">Increment</button>
<p>{count.value}</p>
</template>
```

## Supported APIs
Copy link
Contributor

Choose a reason for hiding this comment

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

How quickly is the code in the README gonna get out of date? 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

This problem isn't unique to @lwc/signals. I think this is fine for now. Long term, I'd love for these sections of the README to be generated from comments + types of the top-level exposed functions/classes/etc.


This package supports the following APIs.

### Signal

This is the shape of the signal that the LWC engine expects.

```js
export type OnUpdate = () => void;
export type Unsubscribe = () => void;

export interface Signal<T> {
get value(): T;
subscribe(onUpdate: OnUpdate): Unsubscribe;
}
```

### SignalBaseClass

A base class is provided as a starting point for implementation.

```js
export abstract class SignalBaseClass<T> implements Signal<T> {
abstract get value(): T;

private subscribers: Set<OnUpdate> = new Set();

subscribe(onUpdate: OnUpdate) {
this.subscribers.add(onUpdate);
return () => {
this.subscribers.delete(onUpdate);
};
}

protected notify() {
for (const subscriber of this.subscribers) {
subscriber();
}
}
}
```
12 changes: 12 additions & 0 deletions packages/@lwc/signals/jest.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/*
* Copyright (c) 2018, 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
*/
const BASE_CONFIG = require('../../../scripts/jest/base.config');

module.exports = {
...BASE_CONFIG,
displayName: 'lwc-signals',
};
44 changes: 44 additions & 0 deletions packages/@lwc/signals/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
{
"//": [
"THIS FILE IS AUTOGENERATED. If you modify it, it will be rewritten by check-and-rewrite-package-json.js",
"You can safely modify dependencies, devDependencies, keywords, etc., but other props will be overwritten."
],
"name": "@lwc/signals",
"version": "6.0.0",
"description": "Provides the protocol to to expose reactivity outside of the LWC framework",
"keywords": [
"lwc"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"lwc"
"lwc",
"signals"

🤷

],
"homepage": "https://lwc.dev",
"repository": {
"type": "git",
"url": "https://github.com/salesforce/lwc.git",
"directory": "packages/@lwc/signals"
},
"bugs": {
"url": "https://github.com/salesforce/lwc/issues"
},
"license": "MIT",
"publishConfig": {
"access": "public"
},
"main": "dist/index.cjs.js",
"module": "dist/index.js",
"types": "dist/index.d.ts",
"files": [
"dist"
],
"scripts": {
"build": "rollup --config ../../../scripts/rollup/rollup.config.js",
"dev": "rollup --config ../../../scripts/rollup/rollup.config.js --watch --no-watch.clearScreen"
},
"nx": {
"targets": {
"build": {
"outputs": [
"{projectRoot}/dist"
]
}
}
}
}
Loading