Skip to content

Commit 82a6410

Browse files
isTrustedSignalshould return false when no trusted set has been defined (#5500)
* fix: signals and tests * fix: add global test fn for signals * chore: because will
1 parent 9ec20a2 commit 82a6410

File tree

10 files changed

+231
-9
lines changed

10 files changed

+231
-9
lines changed

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

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
* SPDX-License-Identifier: MIT
55
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
66
*/
7-
import { isNull, isObject, isTrustedSignal } from '@lwc/shared';
7+
import { isNull, isObject, isTrustedSignal, legacyIsTrustedSignal } from '@lwc/shared';
88
import { ReactiveObserver, valueMutated, valueObserved } from '../libs/mutation-tracker';
99
import { subscribeToSignal } from '../libs/signal-tracker';
1010
import type { Signal } from '@lwc/signals';
@@ -41,13 +41,26 @@ export function componentValueObserved(vm: VM, key: PropertyKey, target: any = {
4141
lwcRuntimeFlags.ENABLE_EXPERIMENTAL_SIGNALS &&
4242
isObject(target) &&
4343
!isNull(target) &&
44-
isTrustedSignal(target) &&
4544
process.env.IS_BROWSER &&
4645
// Only subscribe if a template is being rendered by the engine
4746
tro.isObserving()
4847
) {
49-
// Subscribe the template reactive observer's notify method, which will mark the vm as dirty and schedule hydration.
50-
subscribeToSignal(component, target as Signal<unknown>, tro.notify.bind(tro));
48+
/**
49+
* The legacy validation behavior was that this check should only
50+
* be performed for runtimes that have provided a trustedSignals set.
51+
* However, this resulted in a bug as all object values were
52+
* being considered signals in environments where the trustedSignals
53+
* set had not been defined. The runtime flag has been added as a killswitch
54+
* in case the fix needs to be reverted.
55+
*/
56+
if (
57+
lwcRuntimeFlags.ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION
58+
? legacyIsTrustedSignal(target)
59+
: isTrustedSignal(target)
60+
) {
61+
// Subscribe the template reactive observer's notify method, which will mark the vm as dirty and schedule hydration.
62+
subscribeToSignal(component, target as Signal<unknown>, tro.notify.bind(tro));
63+
}
5164
}
5265
}
5366

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ const features: FeatureFlagMap = {
1919
ENABLE_LEGACY_SCOPE_TOKENS: null,
2020
ENABLE_FORCE_SHADOW_MIGRATE_MODE: null,
2121
ENABLE_EXPERIMENTAL_SIGNALS: null,
22+
ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION: null,
2223
DISABLE_SYNTHETIC_SHADOW: null,
2324
DISABLE_SCOPE_TOKEN_VALIDATION: null,
2425
LEGACY_LOCKER_ENABLED: null,

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,13 @@ export interface FeatureFlagMap {
7070
*/
7171
ENABLE_EXPERIMENTAL_SIGNALS: FeatureFlagValue;
7272

73+
/**
74+
* If true, legacy signal validation is used, where all component properties are considered signals or context
75+
* if a trustedSignalSet and trustedContextSet have not been provided via setTrustedSignalSet and setTrustedContextSet.
76+
* This is a killswitch for a bug fix: #5492
77+
*/
78+
ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION: FeatureFlagValue;
79+
7380
/**
7481
* If true, ignore `@lwc/synthetic-shadow` even if it's loaded on the page. Instead, run all components in
7582
* native shadow mode.
Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
import { createElement, setFeatureFlagForTest } from 'lwc';
2+
3+
import Test from 'x/test';
4+
import { Signal } from 'x/signal';
5+
import { spyConsole } from '../../../helpers/console.js';
6+
7+
const createElementSignalAndInsertIntoDom = async (object) => {
8+
const elm = createElement('x-test', { is: Test });
9+
elm.object = object;
10+
document.body.appendChild(elm);
11+
await Promise.resolve();
12+
return elm;
13+
};
14+
15+
describe('signal reaction in lwc', () => {
16+
let consoleSpy;
17+
18+
beforeAll(() => setFeatureFlagForTest('ENABLE_EXPERIMENTAL_SIGNALS', true));
19+
afterAll(() => setFeatureFlagForTest('ENABLE_EXPERIMENTAL_SIGNALS', false));
20+
beforeEach(() => (consoleSpy = spyConsole()));
21+
afterEach(() => consoleSpy.reset());
22+
23+
describe('with trusted signal set', () => {
24+
describe('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION is enabled', () => {
25+
beforeAll(() => setFeatureFlagForTest('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', true));
26+
afterAll(() => setFeatureFlagForTest('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', false));
27+
it('will not warn if rendering non-signal objects ', async () => {
28+
const elm = await createElementSignalAndInsertIntoDom({
29+
value: 'non signal value',
30+
});
31+
expect(consoleSpy.calls.warn.length).toEqual(0);
32+
expect(elm.shadowRoot.textContent).toBe('non signal value');
33+
});
34+
it('will not warn if rendering signal objects', async () => {
35+
const signal = new Signal('signal value');
36+
const elm = await createElementSignalAndInsertIntoDom(signal);
37+
expect(consoleSpy.calls.warn.length).toEqual(0);
38+
signal.value = 'new signal value';
39+
await Promise.resolve();
40+
expect(elm.shadowRoot.textContent).toBe('new signal value');
41+
});
42+
});
43+
44+
describe('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION is disabled', () => {
45+
beforeAll(() =>
46+
setFeatureFlagForTest('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', false)
47+
);
48+
it('will not warn if rendering non-signal objects', async () => {
49+
const elm = await createElementSignalAndInsertIntoDom({
50+
value: 'non signal value',
51+
});
52+
expect(consoleSpy.calls.warn.length).toEqual(0);
53+
expect(elm.shadowRoot.textContent).toBe('non signal value');
54+
});
55+
it('will not warn if rendering signal objects', async () => {
56+
const signal = new Signal('signal value');
57+
const elm = await createElementSignalAndInsertIntoDom(signal);
58+
expect(consoleSpy.calls.warn.length).toEqual(0);
59+
signal.value = 'new signal value';
60+
await Promise.resolve();
61+
expect(elm.shadowRoot.textContent).toBe('new signal value');
62+
});
63+
});
64+
});
65+
66+
describe('without trusted signal set', () => {
67+
beforeAll(() => globalThis.__lwcResetTrustedSignalsSetForTest());
68+
describe('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION is enabled', () => {
69+
beforeAll(() => setFeatureFlagForTest('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', true));
70+
afterAll(() => setFeatureFlagForTest('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', false));
71+
/**
72+
* The legacy validation behavior was that this check should only
73+
* be performed for runtimes that have provided a trustedSignals set.
74+
* However, this resulted in a bug as all object values were
75+
* being considered signals in environments where the trustedSignals
76+
* set had not been defined. The runtime flag has been added as a killswitch
77+
* in case the fix needs to be reverted.
78+
*/
79+
it('will warn if rendering non-signal objects ', async () => {
80+
const elm = await createElementSignalAndInsertIntoDom({
81+
value: 'non signal value',
82+
});
83+
expect(consoleSpy.calls.warn[0][0].message).toContain(
84+
'Attempted to subscribe to an object that has the shape of a signal but received the following error: TypeError: signal.subscribe is not a function'
85+
);
86+
expect(elm.shadowRoot.textContent).toBe('non signal value');
87+
});
88+
it('will not warn if rendering signal objects', async () => {
89+
const signal = new Signal('signal value');
90+
const elm = await createElementSignalAndInsertIntoDom(signal);
91+
expect(consoleSpy.calls.warn.length).toEqual(0);
92+
signal.value = 'new signal value';
93+
await Promise.resolve();
94+
expect(elm.shadowRoot.textContent).toBe('new signal value');
95+
});
96+
});
97+
98+
describe('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION is disabled', () => {
99+
beforeAll(() =>
100+
setFeatureFlagForTest('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', false)
101+
);
102+
it('will not warn if rendering non-signal objects', async () => {
103+
const elm = await createElementSignalAndInsertIntoDom({
104+
value: 'non signal value',
105+
});
106+
expect(consoleSpy.calls.warn.length).toEqual(0);
107+
expect(elm.shadowRoot.textContent).toBe('non signal value');
108+
});
109+
/**
110+
* Signals are designed to be used where trustedSignalSet has been defined via setTrustedSignalSet
111+
* This is because checking against the set is an efficient way to determine if an object is a Signal
112+
* This is acceptable as Signals is an internal API and designed to work where setTrustedSignalSet has been used.
113+
* Because of this, the signal value does not change here.
114+
* See #5347 for context.
115+
*/
116+
it('will not warn if rendering signal objects but it will not react', async () => {
117+
const signal = new Signal('signal value');
118+
const elm = await createElementSignalAndInsertIntoDom(signal);
119+
expect(consoleSpy.calls.warn.length).toEqual(0);
120+
signal.value = 'new signal value';
121+
await Promise.resolve();
122+
expect(elm.shadowRoot.textContent).toBe('signal value');
123+
});
124+
});
125+
});
126+
});
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
// Note for testing purposes the signal implementation uses LWC module resolution to simplify things.
2+
// In production the signal will come from a 3rd party library.
3+
4+
import { addTrustedSignal } from '../../../../../helpers/signals.js';
5+
6+
export class Signal {
7+
subscribers = new Set();
8+
9+
constructor(initialValue) {
10+
this._value = initialValue;
11+
addTrustedSignal(this);
12+
}
13+
14+
set value(newValue) {
15+
this._value = newValue;
16+
this.notify();
17+
}
18+
19+
get value() {
20+
return this._value;
21+
}
22+
23+
subscribe(onUpdate) {
24+
this.subscribers.add(onUpdate);
25+
return () => {
26+
this.subscribers.delete(onUpdate);
27+
};
28+
}
29+
30+
notify() {
31+
for (const subscriber of this.subscribers) {
32+
subscriber();
33+
}
34+
}
35+
36+
getSubscriberCount() {
37+
return this.subscribers.size;
38+
}
39+
}
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
<template>
2+
{object.value}
3+
</template>
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
import { LightningElement, api } from 'lwc';
2+
3+
export default class Test extends LightningElement {
4+
@api object;
5+
}

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,15 @@ describe('signals', () => {
1010
let setTrustedSignalSet: (signals: WeakSet<object>) => void;
1111
let addTrustedSignal: (signal: object) => void;
1212
let isTrustedSignal: (target: object) => boolean;
13+
let legacyIsTrustedSignal: (target: object) => boolean;
1314

1415
beforeEach(async () => {
1516
vi.resetModules();
1617
const signalsModule = await import('../signals');
1718
setTrustedSignalSet = signalsModule.setTrustedSignalSet;
1819
addTrustedSignal = signalsModule.addTrustedSignal;
1920
isTrustedSignal = signalsModule.isTrustedSignal;
21+
legacyIsTrustedSignal = signalsModule.legacyIsTrustedSignal;
2022
});
2123

2224
describe('setTrustedSignalSet', () => {
@@ -53,8 +55,12 @@ describe('signals', () => {
5355
expect(isTrustedSignal({})).toBe(false);
5456
});
5557

56-
it('should return true for all calls when trustedSignals is not set', () => {
57-
expect(isTrustedSignal({})).toBe(true);
58+
it('should return false for all calls when trustedSignals is not set', () => {
59+
expect(isTrustedSignal({})).toBe(false);
60+
});
61+
62+
it('legacyIsTrustedSignal should return true when trustedSignals is not set', () => {
63+
expect(legacyIsTrustedSignal({})).toBe(true);
5864
});
5965
});
6066
});

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

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,20 +6,35 @@
66
*/
77
import { isFalse } from './assert';
88

9-
let trustedSignals: WeakSet<object>;
9+
let trustedSignals: WeakSet<object> | undefined;
1010

1111
export function setTrustedSignalSet(signals: WeakSet<object>) {
1212
isFalse(trustedSignals, 'Trusted Signal Set is already set!');
1313

1414
trustedSignals = signals;
15+
16+
// Only used in LWC's Karma. Contained within the set function as there are multiple imports of
17+
// this module. Placing it here ensures we reference the import where the trustedSignals set is maintained
18+
if (process.env.NODE_ENV === 'test-karma-lwc') {
19+
// Used to reset the global state between test runs
20+
(globalThis as any).__lwcResetTrustedSignalsSetForTest = () => (trustedSignals = undefined);
21+
}
1522
}
1623

1724
export function addTrustedSignal(signal: object) {
1825
// This should be a no-op when the trustedSignals set isn't set by runtime
1926
trustedSignals?.add(signal);
2027
}
2128

22-
export function isTrustedSignal(target: object): boolean {
29+
/**
30+
* The legacy validation behavior was that this check should only
31+
* be performed for runtimes that have provided a trustedSignals set.
32+
* However, this resulted in a bug as all object values were
33+
* being considered signals in environments where the trustedSignals
34+
* set had not been defined. The runtime flag has been added as a killswitch
35+
* in case the fix needs to be reverted.
36+
*/
37+
export function legacyIsTrustedSignal(target: object): boolean {
2338
if (!trustedSignals) {
2439
// The runtime didn't set a trustedSignals set
2540
// this check should only be performed for runtimes that care about filtering signals to track
@@ -28,3 +43,10 @@ export function isTrustedSignal(target: object): boolean {
2843
}
2944
return trustedSignals.has(target);
3045
}
46+
47+
export function isTrustedSignal(target: object): boolean {
48+
if (!trustedSignals) {
49+
return false;
50+
}
51+
return trustedSignals.has(target);
52+
}

scripts/bundlesize/bundlesize.config.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
"files": [
33
{
44
"path": "packages/@lwc/engine-dom/dist/index.js",
5-
"maxSize": "25.0KB"
5+
"maxSize": "24.72KB"
66
},
77
{
88
"path": "packages/@lwc/synthetic-shadow/dist/index.js",

0 commit comments

Comments
 (0)