feat: signals implementation v1 by jmsjtu · Pull Request #3963 · salesforce/lwc

jmsjtu · 2024-01-27T02:39:11Z

Description

This is an initial implementation of the signals RFC with the following deviations:

The LWC engine will attempt to subscribe to the signal when it has been bound to an LWC class and when it is used on a template.
The .value doesn't have to be used on the template, it could simply be the signal itself, although, it will render as [object object].

Branch with the playground updated for signals.

Detailed Design

The engine will attempt to subscribe to a signal if it is attached to an LWC and if it's used on a template.

Signals bound to an LWC

Mirroring the existing reactivity, we use the getter on the LWC to determine if the value resembles the shape of a signal as defined in @lwc/signals.

The callback that the engine subscribes is the re-render callback assigned to the template reactive observer.

const { isDirty } = vm;
if (isFalse(isDirty)) {
    markComponentAsDirty(vm);
    scheduleRehydration(vm);
}

Every time the template re-renders, we call the unsubscribe callback as part of the clean up process. This coincides with whenever the template reactive observer is reset.

Signals used on a template

Subscription to signals occurs during the process of generating the vnodes, for example:

function tmpl($api, $cmp, $slotset, $ctx) {
  const {d: api_dynamic_text, t: api_text, h: api_element} = $api;
  return [api_element("h1", stc0, [api_text("this is the signal value: " + api_dynamic_text($cmp.signal.value))])];
  /*LWC compiler v6.0.0*/
}

Notice the $cmp.signal.value, the getter is accessed at this moment and the routine runs to subscribe.

Does this pull request introduce a breaking change?

✅ No, it does not introduce a breaking change.

All changes are gated behind a feature flag.

Does this pull request introduce an observable change?

✅ No, it does not introduce an observable change.

All changes are gated behind a feature flag.

GUS work item

W-14560945

packages/@lwc/engine-core/src/libs/mutation-tracker/index.ts

packages/@lwc/engine-core/src/framework/vm.ts

divmain · 2024-01-31T05:23:40Z

packages/@lwc/engine-core/src/framework/mutation-tracker.ts

+        'value' in target &&
+        'subscribe' in target &&
+        typeof target.subscribe === 'function'
+    ) {


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

packages/@lwc/engine-core/src/libs/mutation-tracker/index.ts

divmain · 2024-01-31T05:36:26Z

packages/@lwc/engine-core/src/libs/signal/index.ts

+            subscriber();
+        }
+    }
+}


As we discussed, I'd love for this to be moved to a new top-level @lwc/signals package that exposes the types and base class. I would have recommended moving to @lwc/shared, except that is intended as an internal package with other @lwc/* packages being the only direct consumers. In contrast, the signals type and base call may be re-used across the SF ecosystem.

@divmain I moved everything to a new package and added a readme.

If you get a chance, can you look through the readme too?

jmsjtu · 2024-02-02T00:53:08Z

packages/@lwc/signals/README.md

+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.


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!

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.

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

jmsjtu · 2024-02-02T01:23:02Z

packages/@lwc/engine-core/src/libs/signal-tracker/index.ts

+ * to the same signal. Additionally, it keeps track of all signal unsubscribe callbacks, handles invoking
+ * them when necessary and discarding them.
+ */
+class SignalTracker {


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.

jmsjtu · 2024-02-02T01:28:10Z

packages/@lwc/engine-core/src/framework/mutation-tracker.ts

+        tro.isObserving()
+    ) {
+        // 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));


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.

Suggested change

subscribeToSignal(component, target, tro.notify.bind(tro));

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

@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.

packages/@lwc/features/src/index.ts

packages/@lwc/engine-core/src/framework/component.ts

wjhsf · 2024-02-02T18:38:03Z

packages/@lwc/signals/package.json

+    "version": "6.0.0",
+    "description": "Provides the protocol to to expose reactivity outside of the LWC framework",
+    "keywords": [
+        "lwc"


Suggested change

"lwc"

"lwc",

"signals"

🤷

wjhsf · 2024-02-02T18:38:37Z

scripts/bundlesize/bundlesize.config.json

        {
            "path": "packages/@lwc/engine-dom/dist/index.js",
-            "maxSize": "22KB"
+            "maxSize": "24KB"


wjhsf · 2024-02-02T21:16:10Z

packages/@lwc/engine-core/package.json

    "devDependencies": {
-        "observable-membrane": "2.0.0"
+        "observable-membrane": "2.0.0",
+        "@lwc/signals": "6.0.0"


This should be a dependency, or else it's gonna break things! (Alternatively, make the imports in mutation-tracker / signal-tracker conditional.)

@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.

packages/@lwc/engine-core/src/libs/signal-tracker/index.ts

wjhsf · 2024-02-02T21:29:45Z

packages/@lwc/signals/README.md

+</template>
+```
+
+## Supported APIs


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

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.

nolanlawson

LGTM so far, just a few small nitpicks and some suggestions mostly about moving @lwc/signals code to @lwc/engine-core until we decide if we want it to become a real boy or not.

Exciting stuff!

nolanlawson · 2024-02-02T23:20:14Z

packages/@lwc/engine-core/package.json

    "devDependencies": {
-        "observable-membrane": "2.0.0"
+        "observable-membrane": "2.0.0",
+        "@lwc/signals": "6.0.0"


@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.

packages/@lwc/engine-core/src/framework/component.ts

packages/@lwc/engine-core/src/framework/mutation-tracker.ts

nolanlawson · 2024-02-02T23:23:46Z

packages/@lwc/engine-core/src/framework/mutation-tracker.ts

+        target &&
+        typeof target === 'object' &&
+        'value' in target &&
+        'subscribe' in target &&


Suggested change

'subscribe' in target &&

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

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

packages/@lwc/engine-core/src/framework/mutation-tracker.ts

nolanlawson · 2024-02-02T23:29:29Z

packages/@lwc/engine-core/src/libs/signal-tracker/index.ts

+        } catch (err) {
+            logWarnOnce(
+                `Attempted to subscribe to an object that has the shape of a signal but received the following error: ${err}`
+            );


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?

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}`);

@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.

nolanlawson · 2024-02-02T23:30:52Z

packages/@lwc/engine-core/src/libs/signal-tracker/index.ts

+        } catch (err) {
+            logWarnOnce(
+                `Attempted to call a signal's unsubscribe callback but received the following error: ${err}`
+            );


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.

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.

@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.

packages/@lwc/features/src/index.ts

nolanlawson · 2024-02-02T23:32:14Z

packages/@lwc/signals/README.md

+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.


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.

scripts/tasks/check-and-rewrite-package-json.js

divmain

This is looking good. As soon as the minor comments/suggestions are addressed, I think this is ready to merge as our v1.

divmain · 2024-02-05T22:06:33Z

packages/@lwc/engine-core/src/libs/signal-tracker/index.ts

+    subscribeToSignal(signal: Signal<unknown>, update: CallbackFunction) {
+        try {
+            const unsubscribe = signal.subscribe(update);
+            if (typeof unsubscribe === 'function') {


Agree with the above suggested change. Also, the thing is not a signal if typeof subscribe() !== 'function'. The signal contract is being broken in this case. With the current implementation, that'd result in a silent failure. Long-term we may want to surface a warning or error for stronger enforcement. I'd suggest opening an issue to revisit this in 252 once we have a better idea of the best behavior to adopt here.

divmain · 2024-02-05T22:11:45Z

packages/@lwc/engine-core/src/libs/signal-tracker/index.ts

+        } catch (err) {
+            logWarnOnce(
+                `Attempted to subscribe to an object that has the shape of a signal but received the following error: ${err}`
+            );


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}`);

divmain · 2024-02-05T22:15:06Z

packages/@lwc/engine-core/src/libs/signal-tracker/index.ts

+        } catch (err) {
+            logWarnOnce(
+                `Attempted to call a signal's unsubscribe callback but received the following error: ${err}`
+            );


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.

Co-authored-by: Nolan Lawson <nlawson@salesforce.com>

packages/@lwc/signals/tsconfig.json

scripts/jest/root.config.js

nolanlawson · 2024-02-06T22:44:44Z

packages/@lwc/integration-karma/README.md

 -   **`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.


It just occurred to me: we do not technically need a new Karma env var, since ENABLE_EXPERIMENTAL_SIGNALS is a runtime-only flag. So you could just set the flag at runtime during the Karma tests.

Or do you intend to make it a compiler flag too in the future?

TIL! No, I don't intend for there to be a compiler flag in the future.

I've removed it from the karma tests.

nolanlawson

LGTM, a few nits and one question.

feat: initial signal implementation

4a0e5f7

jmsjtu requested review from a team and divmain and removed request for a team January 27, 2024 02:39

jmsjtu added the nomerge label Jan 27, 2024

jmsjtu changed the title ~~feat: signals implementation~~ feat: signals implementation v1 Jan 29, 2024

jmsjtu commented Jan 29, 2024

View reviewed changes

packages/@lwc/engine-core/src/libs/mutation-tracker/index.ts Outdated Show resolved Hide resolved

jmsjtu commented Jan 29, 2024

View reviewed changes

packages/@lwc/engine-core/src/framework/vm.ts Outdated Show resolved Hide resolved

jmsjtu added 3 commits January 30, 2024 14:51

refactor: clean up logic and better use of mutation tracker and observer

f7f658d

chore: add a todo

0a08b45

chore: update mutation tracker logic

66f885b

divmain reviewed Jan 31, 2024

View reviewed changes

jmsjtu added 3 commits February 1, 2024 16:26

refactor: move logic for tracking signal subscription

65fa1cb

chore: move unit tests to @lwc/signals package

8037a6c

chore: bump bundlesize

7b3582f

jmsjtu commented Feb 2, 2024

View reviewed changes

chore: update package.json script

92413fc

jmsjtu commented Feb 2, 2024

View reviewed changes

packages/@lwc/features/src/index.ts Show resolved Hide resolved

jmsjtu commented Feb 2, 2024

View reviewed changes

packages/@lwc/engine-core/src/framework/component.ts Outdated Show resolved Hide resolved

jmsjtu added 2 commits February 1, 2024 17:31

chore: fix package.json

fcdae16

chore: update unsubscribe logic

cf3bafd

jmsjtu marked this pull request as ready for review February 2, 2024 20:35

jmsjtu requested a review from a team as a code owner February 2, 2024 20:35

jmsjtu requested review from divmain and nolanlawson and removed request for a team February 2, 2024 20:35

jmsjtu removed the nomerge label Feb 2, 2024

wjhsf reviewed Feb 2, 2024

View reviewed changes

nolanlawson suggested changes Feb 2, 2024

View reviewed changes

divmain approved these changes Feb 5, 2024

View reviewed changes

jmsjtu and others added 7 commits February 5, 2024 14:29

Update packages/@lwc/engine-core/src/framework/mutation-tracker.ts

b06528b

Co-authored-by: Nolan Lawson <nlawson@salesforce.com>

chore: update based on review feedback

f315852

Merge branch 'master' of github.com:salesforce/lwc into jtu/signals

4ec0439

feat: signals test cases (#3962)

3c78f17

chore: remove rewriting package.json for @lwc/signals

e3b6e82

chore: update package.json

2fa1693

Merge branch 'jtu/signals' of github.com:salesforce/lwc into jtu/signals

1565a04

jmsjtu requested a review from nolanlawson February 6, 2024 22:28

nolanlawson reviewed Feb 6, 2024

View reviewed changes

packages/@lwc/signals/tsconfig.json Outdated Show resolved Hide resolved

nolanlawson reviewed Feb 6, 2024

View reviewed changes

scripts/jest/root.config.js Outdated Show resolved Hide resolved

nolanlawson reviewed Feb 6, 2024

View reviewed changes

nolanlawson approved these changes Feb 6, 2024

View reviewed changes

jmsjtu added 4 commits February 6, 2024 15:25

chore: remove karma test flag

6fde22a

chore: fix native lifecycle error

3c6aa00

chore: styling nit

1565db7

chore: fix spelling issue

6360bb6

jmsjtu merged commit 314ab66 into master Feb 7, 2024

jmsjtu deleted the jtu/signals branch February 7, 2024 00:32

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

Comments

Conversation

jmsjtu commented Jan 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Detailed Design

Signals bound to an LWC

Signals used on a template

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmsjtu Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nolanlawson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmsjtu Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmsjtu commented Jan 27, 2024 •

edited

Loading

jmsjtu Feb 2, 2024 •

edited

Loading

jmsjtu Feb 6, 2024 •

edited

Loading