Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
21 changes: 17 additions & 4 deletions packages/@lwc/engine-core/src/framework/mutation-tracker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 { isNull, isObject, isTrustedSignal, legacyIsTrustedSignal } from '@lwc/shared';
import { ReactiveObserver, valueMutated, valueObserved } from '../libs/mutation-tracker';
import { subscribeToSignal } from '../libs/signal-tracker';
import type { Signal } from '@lwc/signals';
Expand Down Expand Up @@ -41,13 +41,26 @@ export function componentValueObserved(vm: VM, key: PropertyKey, target: any = {
lwcRuntimeFlags.ENABLE_EXPERIMENTAL_SIGNALS &&
isObject(target) &&
!isNull(target) &&
isTrustedSignal(target) &&
process.env.IS_BROWSER &&
// Only subscribe if a template is being rendered by the engine
tro.isObserving()
) {
// Subscribe the template reactive observer's notify method, which will mark the vm as dirty and schedule hydration.
subscribeToSignal(component, target as Signal<unknown>, tro.notify.bind(tro));
/**
* The legacy validation behavior was that this check should only
* be performed for runtimes that have provided a trustedSignals set.
* However, this resulted in a bug as all object values were
* being considered signals in environments where the trustedSignals
* set had not been defined. The runtime flag has been added as a killswitch
* in case the fix needs to be reverted.
*/
if (
lwcRuntimeFlags.ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION
? legacyIsTrustedSignal(target)
: isTrustedSignal(target)
) {
// Subscribe the template reactive observer's notify method, which will mark the vm as dirty and schedule hydration.
subscribeToSignal(component, target as Signal<unknown>, tro.notify.bind(tro));
}
}
}

Expand Down
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_LEGACY_SCOPE_TOKENS: null,
ENABLE_FORCE_SHADOW_MIGRATE_MODE: null,
ENABLE_EXPERIMENTAL_SIGNALS: null,
ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION: null,
DISABLE_SYNTHETIC_SHADOW: null,
DISABLE_SCOPE_TOKEN_VALIDATION: null,
LEGACY_LOCKER_ENABLED: null,
Expand Down
7 changes: 7 additions & 0 deletions packages/@lwc/features/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,13 @@ export interface FeatureFlagMap {
*/
ENABLE_EXPERIMENTAL_SIGNALS: FeatureFlagValue;

/**
* If true, legacy signal validation is used, where all component properties are considered signals or context
* if a trustedSignalSet and trustedContextSet have not been provided via setTrustedSignalSet and setTrustedContextSet.
* This is a killswitch for a bug fix: #5492
*/
ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION: FeatureFlagValue;

/**
* If true, ignore `@lwc/synthetic-shadow` even if it's loaded on the page. Instead, run all components in
* native shadow mode.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
import { createElement, setFeatureFlagForTest } from 'lwc';

import Test from 'x/test';
import { Signal } from 'x/signal';
import { spyConsole } from '../../../helpers/console.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO spyConsole doesn't add any value over just using vitest's spyOn. I want to remove spyConsole eventually, but that's a problem for later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tech debt ftw


const createElementSignalAndInsertIntoDom = async (object) => {
const elm = createElement('x-test', { is: Test });
elm.object = object;
document.body.appendChild(elm);
await Promise.resolve();
return elm;
};

describe('signal reaction in lwc', () => {
let consoleSpy;

beforeAll(() => setFeatureFlagForTest('ENABLE_EXPERIMENTAL_SIGNALS', true));
afterAll(() => setFeatureFlagForTest('ENABLE_EXPERIMENTAL_SIGNALS', false));
beforeEach(() => (consoleSpy = spyConsole()));
afterEach(() => consoleSpy.reset());

describe('with trusted signal set', () => {
describe('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION is enabled', () => {
beforeAll(() => setFeatureFlagForTest('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', true));
afterAll(() => setFeatureFlagForTest('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', false));
it('will not warn if rendering non-signal objects ', async () => {
const elm = await createElementSignalAndInsertIntoDom({
value: 'non signal value',
});
expect(consoleSpy.calls.warn.length).toEqual(0);
expect(elm.shadowRoot.textContent).toBe('non signal value');
});
it('will not warn if rendering signal objects', async () => {
const signal = new Signal('signal value');
const elm = await createElementSignalAndInsertIntoDom(signal);
expect(consoleSpy.calls.warn.length).toEqual(0);
signal.value = 'new signal value';
await Promise.resolve();
expect(elm.shadowRoot.textContent).toBe('new signal value');
});
});

describe('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION is disabled', () => {
beforeAll(() =>
setFeatureFlagForTest('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', false)
);
it('will not warn if rendering non-signal objects', async () => {
const elm = await createElementSignalAndInsertIntoDom({
value: 'non signal value',
});
expect(consoleSpy.calls.warn.length).toEqual(0);
expect(elm.shadowRoot.textContent).toBe('non signal value');
});
it('will not warn if rendering signal objects', async () => {
const signal = new Signal('signal value');
const elm = await createElementSignalAndInsertIntoDom(signal);
expect(consoleSpy.calls.warn.length).toEqual(0);
signal.value = 'new signal value';
await Promise.resolve();
expect(elm.shadowRoot.textContent).toBe('new signal value');
});
});
});

describe('without trusted signal set', () => {
beforeAll(() => globalThis.__lwcResetTrustedSignalsSetForTest());
describe('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION is enabled', () => {
beforeAll(() => setFeatureFlagForTest('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', true));
afterAll(() => setFeatureFlagForTest('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', false));
/**
* The legacy validation behavior was that this check should only
* be performed for runtimes that have provided a trustedSignals set.
* However, this resulted in a bug as all object values were
* being considered signals in environments where the trustedSignals
* set had not been defined. The runtime flag has been added as a killswitch
* in case the fix needs to be reverted.
*/
it('will warn if rendering non-signal objects ', async () => {
const elm = await createElementSignalAndInsertIntoDom({
value: 'non signal value',
});
expect(consoleSpy.calls.warn[0][0].message).toContain(
'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'
);
expect(elm.shadowRoot.textContent).toBe('non signal value');
});
it('will not warn if rendering signal objects', async () => {
const signal = new Signal('signal value');
const elm = await createElementSignalAndInsertIntoDom(signal);
expect(consoleSpy.calls.warn.length).toEqual(0);
signal.value = 'new signal value';
await Promise.resolve();
expect(elm.shadowRoot.textContent).toBe('new signal value');
});
});

describe('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION is disabled', () => {
beforeAll(() =>
setFeatureFlagForTest('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', false)
);
it('will not warn if rendering non-signal objects', async () => {
const elm = await createElementSignalAndInsertIntoDom({
value: 'non signal value',
});
expect(consoleSpy.calls.warn.length).toEqual(0);
expect(elm.shadowRoot.textContent).toBe('non signal value');
});
/**
* Signals are designed to be used where trustedSignalSet has been defined via setTrustedSignalSet
* This is because checking against the set is an efficient way to determine if an object is a Signal
* This is acceptable as Signals is an internal API and designed to work where setTrustedSignalSet has been used.
* Because of this, the signal value does not change here.
* See #5347 for context.
*/
it('will not warn if rendering signal objects but it will not react', async () => {
const signal = new Signal('signal value');
const elm = await createElementSignalAndInsertIntoDom(signal);
expect(consoleSpy.calls.warn.length).toEqual(0);
signal.value = 'new signal value';
await Promise.resolve();
expect(elm.shadowRoot.textContent).toBe('signal value');
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Note for testing purposes the signal implementation uses LWC module resolution to simplify things.
// In production the signal will come from a 3rd party library.

import { addTrustedSignal } from '../../../../../helpers/signals.js';

export class Signal {
subscribers = new Set();

constructor(initialValue) {
this._value = initialValue;
addTrustedSignal(this);
}

set value(newValue) {
this._value = newValue;
this.notify();
}

get value() {
return this._value;
}

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

notify() {
for (const subscriber of this.subscribers) {
subscriber();
}
}

getSubscriberCount() {
return this.subscribers.size;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<template>
{object.value}
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { LightningElement, api } from 'lwc';

export default class Test extends LightningElement {
@api object;
}
10 changes: 8 additions & 2 deletions packages/@lwc/shared/src/__tests__/signals.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@ describe('signals', () => {
let setTrustedSignalSet: (signals: WeakSet<object>) => void;
let addTrustedSignal: (signal: object) => void;
let isTrustedSignal: (target: object) => boolean;
let legacyIsTrustedSignal: (target: object) => boolean;

beforeEach(async () => {
vi.resetModules();
const signalsModule = await import('../signals');
setTrustedSignalSet = signalsModule.setTrustedSignalSet;
addTrustedSignal = signalsModule.addTrustedSignal;
isTrustedSignal = signalsModule.isTrustedSignal;
legacyIsTrustedSignal = signalsModule.legacyIsTrustedSignal;
});

describe('setTrustedSignalSet', () => {
Expand Down Expand Up @@ -53,8 +55,12 @@ describe('signals', () => {
expect(isTrustedSignal({})).toBe(false);
});

it('should return true for all calls when trustedSignals is not set', () => {
expect(isTrustedSignal({})).toBe(true);
it('should return false for all calls when trustedSignals is not set', () => {
expect(isTrustedSignal({})).toBe(false);
});

it('legacyIsTrustedSignal should return true when trustedSignals is not set', () => {
expect(legacyIsTrustedSignal({})).toBe(true);
});
});
});
26 changes: 24 additions & 2 deletions packages/@lwc/shared/src/signals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,35 @@
*/
import { isFalse } from './assert';

let trustedSignals: WeakSet<object>;
let trustedSignals: WeakSet<object> | undefined;

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

trustedSignals = signals;

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

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 {
/**
* The legacy validation behavior was that this check should only
* be performed for runtimes that have provided a trustedSignals set.
* However, this resulted in a bug as all object values were
* being considered signals in environments where the trustedSignals
* set had not been defined. The runtime flag has been added as a killswitch
* in case the fix needs to be reverted.
*/
export function legacyIsTrustedSignal(target: object): boolean {
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
Expand All @@ -28,3 +43,10 @@ export function isTrustedSignal(target: object): boolean {
}
return trustedSignals.has(target);
}

export function isTrustedSignal(target: object): boolean {
if (!trustedSignals) {
return false;
}
return trustedSignals.has(target);
}
2 changes: 1 addition & 1 deletion scripts/bundlesize/bundlesize.config.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"files": [
{
"path": "packages/@lwc/engine-dom/dist/index.js",
"maxSize": "25.0KB"
"maxSize": "24.72KB"
},
{
"path": "packages/@lwc/synthetic-shadow/dist/index.js",
Expand Down