chore: address feedback

Author: rax-it

packages/@lwc/engine-core/README.md

@@ -116,6 +116,8 @@ This experimental API enables the sanitization of HTML content by external servi
 
 This experimental API enables the removal of an object's observable membrane proxy wrapper.
 
-### setSignalValidator()
+### setTrustedSignalSet()
 
-This experimental API enables the addition of a signal as a trusted signal. If the [`ENABLE_EXPERIMENTAL_SIGNALS`](https://github.com/salesforce/lwc/blob/master/packages/%40lwc/features/README.md#lwcfeatures) feature is enabled, any signal value change will trigger a re-render.
+This experimental API enables the addition of a signal as a trusted signal. If the [ENABLE_EXPERIMENTAL_SIGNALS](https://github.com/salesforce/lwc/blob/master/packages/%40lwc/features/README.md#lwcfeatures) feature is enabled, any signal value change will trigger a re-render.
+
+If `setTrustedSignalSet` is called more than once, it will throw an error. If it is never called, then no trusted signal validation will be performed. The same `setTrustedSignalSet` API must be called on both `@lwc/engine-dom` and `@lwc/signals`.

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

@@ -4,7 +4,7 @@
  * SPDX-License-Identifier: MIT
  * For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
  */
-import { isNull, isObject, isTrustedSignal } from '@lwc/shared';
+import { isFunction, isNull, isObject, isTrustedSignal } from '@lwc/shared';
 import { Signal } from '@lwc/signals';
 import {
     JobFunction,
@@ -46,6 +46,9 @@ export function componentValueObserved(vm: VM, key: PropertyKey, target: any = {
         lwcRuntimeFlags.ENABLE_EXPERIMENTAL_SIGNALS &&
         isObject(target) &&
         !isNull(target) &&
+        'value' in target &&
+        'subscribe' in target &&
+        isFunction(target.subscribe) &&
         isTrustedSignal(target) &&
         // Only subscribe if a template is being rendered by the engine
         tro.isObserving()

packages/@lwc/integration-karma/test/signal/protocol/index.spec.js

@@ -183,6 +183,18 @@ describe('signal protocol', () => {
         expect(elm.getSignalRemovedSubscriberCount()).toBe(2);
     });
 
+    it('does not subscribe if the signal shape is incorrect', async () => {
+        const elm = createElement('x-child', { is: Child });
+        const subscribe = jasmine.createSpy();
+        // Note the signals property is value's' and not value
+        const signal = { values: 'initial value', subscribe };
+        elm.signal = signal;
+        document.body.appendChild(elm);
+        await Promise.resolve();
+
+        expect(subscribe).not.toHaveBeenCalled();
+    });
+
     it('does not subscribe if the signal is not added as trusted signal', async () => {
         const elm = createElement('x-child', { is: Child });
         const subscribe = jasmine.createSpy();

packages/@lwc/shared/src/__tests__/signals.spec.ts

@@ -0,0 +1,60 @@
+/*
+ * 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 { vi } from 'vitest';
+
+describe('signals', () => {
+    let setTrustedSignalSet: (signals: WeakSet<object>) => void;
+    let addTrustedSignal: (signal: object) => void;
+    let isTrustedSignal: (target: object) => boolean;
+
+    beforeEach(async () => {
+        vi.resetModules();
+        const signalsModule = await import('../signals');
+        setTrustedSignalSet = signalsModule.setTrustedSignalSet;
+        addTrustedSignal = signalsModule.addTrustedSignal;
+        isTrustedSignal = signalsModule.isTrustedSignal;
+    });
+
+    describe('setTrustedSignalSet', () => {
+        it('should throw an error if trustedSignals is already set', () => {
+            setTrustedSignalSet(new WeakSet());
+            expect(() => setTrustedSignalSet(new WeakSet())).toThrow(
+                'Trusted Signal Set is already set!'
+            );
+        });
+    });
+
+    describe('addTrustedSignal', () => {
+        it('should add a signal to the trustedSignals set', () => {
+            const mockWeakSet = new WeakSet();
+            setTrustedSignalSet(mockWeakSet);
+            const signal = {};
+            addTrustedSignal(signal);
+            expect(isTrustedSignal(signal)).toBe(true);
+        });
+    });
+
+    describe('isTrustedSignal', () => {
+        it('should return true for a trusted signal', () => {
+            const mockWeakSet = new WeakSet();
+            setTrustedSignalSet(mockWeakSet);
+            const signal = {};
+            addTrustedSignal(signal);
+            expect(isTrustedSignal(signal)).toBe(true);
+        });
+
+        it('should return false for an untrusted signal', () => {
+            const mockWeakSet = new WeakSet();
+            setTrustedSignalSet(mockWeakSet);
+            expect(isTrustedSignal({})).toBe(false);
+        });
+
+        it('should return true for all calls when trustedSignals is not set', () => {
+            expect(isTrustedSignal({})).toBe(true);
+        });
+    });
+});

packages/@lwc/shared/src/signals.ts

@@ -8,16 +8,23 @@ import { isFalse } from './assert';
 
 let trustedSignals: WeakSet<object>;
 
-export function setSignalValidator(signals: WeakSet<object>) {
-    isFalse(trustedSignals, 'Already added a signal validator');
+export function setTrustedSignalSet(signals: WeakSet<object>) {
+    isFalse(trustedSignals, 'Trusted Signal Set is already set!');
 
     trustedSignals = signals;
 }
 
 export function addTrustedSignal(signal: object) {
+    // This should be a no-op when the trustedSignals set isn't set by runtime
     trustedSignals?.add(signal);
 }
 
 export function isTrustedSignal(target: object): boolean {
-    return trustedSignals?.has(target);
+    if (!trustedSignals) {
+        // The runtime didn't set a trustedSignals set
+        // this check should only be performed for runtimes that care about filtering signals to track
+        // our default behavior should be to track all signals
+        return true;
+    }
+    return trustedSignals.has(target);
 }