isTrustedSignal/Context should return false when no trusted set has been defined by jhefferman-sfdc · Pull Request #5492 · salesforce/lwc

jhefferman-sfdc · 2025-09-12T19:53:46Z

Details

An internal Signals API is not being called in this context, as state managers are not in use. When the internal API is not called, all objects are considered signals and a warning is logged for properties that do not meet the criteria.

Does this pull request introduce a breaking change?

😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

🤞 No, it does not introduce an observable change.

GUS work item

W-19640386

…sted set

wjhsf · 2025-09-12T22:17:57Z

😮‍💨 No, it does not introduce a breaking change.

Pretty sure changing "this returns true" to "this returns false" is definitely a breaking change.

Also, why?

jhefferman-sfdc · 2025-09-13T02:02:07Z

😮‍💨 No, it does not introduce a breaking change.

Pretty sure changing "this returns true" to "this returns false" is definitely a breaking change.

Explain what makes you sure of this?

Also, why?

Please see my thorough explanation in the description (p.s. thank you for the inspiration)

wjhsf · 2025-09-15T18:37:37Z

If an OSS user wants to use alternative signal implementations with LWC, they can currently do that by simply not calling setTrustedSignalSet. With this change, not calling setTrustedSignalSet means no signals work, while calling setTrustedSignalSet makes only trusted signals work. There's no way to allow alternative signal implementations. Is that something that we care about?

If we do care about it, my proposal is to update the function signature to setTrustedSignalSet(signals: WeakSet<object> | true). If the user passes true, then all signals are trusted.

As part of integrating signals into the framework, we added the concept of "trusted" signals in #4665. This restricted the framework to only use signals from @lwc/signals, rather than allowing any third-party implementation. To avoid breaking users already consuming signals, the "trusted" mechanism was opt-in. The helper function setTrustedSignalSet must be called to track trusted vs untrusted signals, otherwise all signals are considered trusted. When the framework checked components for mutations, checking for a "trusted" signal was added as a check after validating that the mutated value was a signal.

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

Lines 45 to 58 in f2583dc

    
           if ( 
        
               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() 
        
           ) { 
        
               // 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)); 
        
           }

Later, a performance regression related to the in operator was discovered, so those checks were removed in #5347.

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

Lines 40 to 50 in 98a4d69

    
           if ( 
        
               lwcRuntimeFlags.ENABLE_EXPERIMENTAL_SIGNALS && 
        
               isObject(target) && 
        
               !isNull(target) && 
        
               isTrustedSignal(target) && 
        
               // 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)); 
        
           }

However, this had an unintended consequence. If setTrustedSignalSet is not called, every signal is subscribed to. However, because the shape of the mutated object is not checked to be a signal, the framework attempts to subscribe to every mutated object. Subscribing to the signal is wrapped in a try/catch, so everything still works, but it's a performance regression and adds an extreme amount of noise to the console.

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

Lines 60 to 75 in 9e4d3fa

    
               subscribeToSignal(signal: Signal<unknown>, update: CallbackFunction) { 
        
                   try { 
        
                       const unsubscribe = signal.subscribe(update); 
        
                       if (isFunction(unsubscribe)) { 
        
                           // TODO [#3978]: Evaluate how we should handle the case when unsubscribe is not a function. 
        
                           // Long term we should throw an error or log a warning. 
        
                           this.signalToUnsubscribeMap.set(signal, unsubscribe); 
        
                       } 
        
                   } catch (err: any) { 
        
                       logWarnOnce( 
        
                           `Attempted to subscribe to an object that has the shape of a signal but received the following error: ${ 
        
                               err?.stack ?? err 
        
                           }` 
        
                       ); 
        
                   } 
        
               }

Ideally, in order to only subscribe to actual signals, we would check the shape of the mutated object. Unfortunately, due to performance issues, that is not a viable approach. An alternative is to change the behavior of the "trusted" mechanism. Everything works correctly when setTrustedSignalSet is called, so we could make that a requirement when initializing the framework. However, there may be users who do not use signals, and we don't want to require them to configure something they won't use. Another option is to change the behavior from trusting everything when unset to trusting nothing. This has no impact on users who use trusted signals, and fixes the performance/logging issues for users who do not use signals. However, it changes the behavior for users who use signals, but not the "trusted" mechanism. Before the change, all signals work, from @lwc/signals or any other library. After the change, no signals work, which is a breaking change. The fix for trusted signals is relatively straightforward, they just need to call setTrustedSignalSet with a WeakMap. For non-trusted signals, they would need to do extra work to manage the trusted WeakMap themselves.

Even though this is technically a breaking change, the signals feature is considered a subset of state management, which is not yet released. Additionally, we don't know of any users that are using non-trusted signals with LWC. Therefore, this seems like a reasonably safe breaking change to make.

wjhsf · 2025-09-15T18:51:18Z

This is a pretty minor thought on a feature that's already shipped, so it's probably not worth doing. 😞

import * as lwc from 'lwc'
import * as lwcSignals from '@lwc/signals'
const signals = new WeakMap()
lwc.setTrustedSignalSet(signals)
//  ^? function setTrustedSignalSet(signals: WeakSet<object>): void;
lwcSignals.setTrustedSignalSet(signals)
//  ^? function setTrustedSignalSet(signals: WeakSet<object>): void;

While we're at it, the name setTrustedSignalSet is kludgy And the usage isn't very intuitive. Why do I need to create a WeakSet? What is a "trusted signal set"? Why should I care?

import * as lwc from 'lwc'
import * as lwcSignals from 'lwcSignals'
lwc.enableSignals( //  => function enableSignals(signalTracker: WeakMap<object>): void;
  lwcSignals.createSignalTracker() // => function createSignalTracker(): WeakMap<object>;
)

Something like this feels a lot more ergonomic. The goal is clear (enable signals). The name "signal tracker" doesn't explain everything, but it's gives more of an idea than "trusted signal set". And with the changed signature, it's not quite entirely hidden, but the user doesn't have to create it or manage it on their own. However, if they want to, they still can!

wjhsf · 2025-09-15T18:52:10Z

@jhefferman-sfdc do you remember if 'subscribe' in target was the only perf bomb, or if typeof target.subscribe === 'function' also slowed things down? If we can use typeof, then we could do some shape checking...

wjhsf

Can you add some tests that validate that nothing is logged to console when a non-signal object is mutation tracked?

wjhsf · 2025-09-15T20:33:05Z

scripts/bundlesize/bundlesize.config.json

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


wjhsf · 2025-09-15T20:42:23Z

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

+            (lwcRuntimeFlags.ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION &&
+                legacyIsTrustedSignal(target)) ||
+            (!lwcRuntimeFlags.ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION && isTrustedSignal(target))


Suggested change

(lwcRuntimeFlags.ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION &&

legacyIsTrustedSignal(target)) ||

(!lwcRuntimeFlags.ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION && isTrustedSignal(target))

lwcRuntimeFlags.ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION &&

? legacyIsTrustedSignal(target)

: isTrustedSignal(target)

bruv

I had a slight preference for the verbose syntax but on second thoughts, you're right

jmsjtu · 2025-09-15T20:36:51Z

packages/@lwc/engine-core/src/framework/modules/context.ts

+        lwcRuntimeFlags.ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION
+            ? legacyIsTrustedContext((component as any)[enumerableKey])
+            : isTrustedContext((component as any)[enumerableKey])
    );


@jhefferman-sfdc - Just to confirm, this is how it will work with the killswitch right?

killswitch on => ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION is off

killswitch off => ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION is on

@jmsjtu ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION is the kill switch so:

killswitch on => ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION is on => use current legacy validation which were are fixing (legacyIsTrustedSignal/Context)

killswitch off => ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION is off => use corrected validation (isTrustedSignal/Context)

Is that OK / makes sense?

@jmsjtu you are correct, but I think we will have to invert the flag in core as our default flag state is undefined and we want the default behavior to be the corrected behavior? Aka what you said:

killswitch on => ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION is off

killswitch off => ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION is on

jmsjtu · 2025-09-15T20:54:58Z

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

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


For posterity, can we add a test that shows the legacy vs new beavior?

Ex, a test where the flag is enabled vs disabled and the expected outputs.

Good idea, I've added some tests around this change.

wjhsf · 2025-09-15T20:46:02Z

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

+/**
+ * The legacy validation behavior was that this check should only
+ * be performed for runtimes that have provided a trustedContext set.
+ * However, this resulted in a bug as all component properties were


nit: technically it was only object values, not all values

jhefferman-sfdc · 2025-09-16T00:30:56Z

@wjhsf :

@jhefferman-sfdc do you remember if 'subscribe' in target was the only perf bomb, or if typeof target.subscribe === 'function' also slowed things down? If we can use typeof, then we could do some shape checking...

Interesting concept but I'd rather not rock the boat at this stage. Gonna stick with Dale's proposed WeakSet approach.

This is a pretty minor thought on a feature that's already shipped, so it's probably not worth doing. 😞

It does seem clearer but it will change the contract with the state managers and they've already baked this into their codebase. Perhaps we can review if we do another round of improvements with them.

To avoid breaking users already consuming signals, the "trusted" mechanism was opt-in.

Thanks for your problem summary. However the main reasoning you mention here appears to be an edge case because:

Signal tracking was only implemented 19 months ago and was not documented anywhere except readme, as an internal-only feature.
The feature was behind default-off. experimental flag for that time
If signals had been turned on and used despite not being documented, OSS users would have had this console/throwing error too for every single non-signal object value on every single component for the last many months.
Considering the console logging was reported almost straight away in non-OSS, I highly doubt this change will impact any active OSS users of Signals but even it did, they just need to call setTrustedSignal/Context set.

jhefferman-sfdc · 2025-09-16T14:50:08Z

/nucleus ignore -m "unrelated failures"

jmsjtu · 2025-09-16T17:47:42Z

packages/@lwc/engine-core/src/framework/__tests__/context.spec.ts

+vi.mock('../../shared/logger', () => ({
+    logWarnOnce: vi.fn(),
+}));


We have some helper methods defined already for the loggers you could use.

jmsjtu · 2025-09-16T17:49:37Z

packages/@lwc/engine-core/src/framework/__tests__/context.spec.ts

+const setFeatureFlag = (name: string, value: boolean) => {
+    (globalThis as any).lwcRuntimeFlags[name] = value;
+};


You can re-use the setFeatureFlagsForTest helper instead of defining your own.

jmsjtu · 2025-09-16T17:58:40Z

packages/@lwc/engine-core/src/framework/__tests__/context.spec.ts

+    describe('without setting trusted context', () => {
+        it('should log a warning when trustedContext is not defined and connectContext is called with legacy signal context validation', () => {
+            setFeatureFlag('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', true);
+            connectContext(mockVM);
+            expect(logWarnOnce).toHaveBeenCalledWith(
+                'Attempted to connect to trusted context but received the following error: component[contextfulKeys[i]][connectContext2] is not a function'
+            );
+        });


Instead of creating these two tests in context/mutation-tracker how would you feel if we created a single test using an actual LWC to replicate the before and after behavior?

Is it possible to toggle the trusted signal set in that way?

fix: isTrustedSignal/Context should return false when there is no tru…

26e22d0

…sted set

jhefferman-sfdc requested a review from a team as a code owner September 12, 2025 19:53

salesforce deleted a comment from wjhsf Sep 15, 2025

wjhsf reviewed Sep 15, 2025

View reviewed changes

jhefferman-sfdc added 3 commits September 15, 2025 14:08

fix: add killswitch

5973cc7

fix: bundlesize

f862a2b

fix: bundlesize

0e8cdd4

wjhsf reviewed Sep 15, 2025

View reviewed changes

jmsjtu reviewed Sep 15, 2025

View reviewed changes

wjhsf reviewed Sep 15, 2025

View reviewed changes

jhefferman-sfdc added 2 commits September 15, 2025 18:23

fix: review comments

0804c69

fix: test coverage for signal/context validation behavior

ed9c66a

jhefferman-sfdc added 2 commits September 15, 2025 21:24

fix: vitest prod mode compat

8be0a75

fix: test typos

550da05

jhefferman-sfdc requested review from jmsjtu and wjhsf September 16, 2025 03:33

fix: missing test

2734268

wjhsf mentioned this pull request Sep 16, 2025

chore: remove perf regression for signals re-enablement #5347

Merged

jmsjtu reviewed Sep 16, 2025

View reviewed changes

jhefferman-sfdc closed this Sep 17, 2025

ekashida mentioned this pull request Sep 18, 2025

isTrustedSignalshould return false when no trusted set has been defined #5500

Merged

Conversation

jhefferman-sfdc commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

Uh oh!

wjhsf commented Sep 12, 2025

Uh oh!

jhefferman-sfdc commented Sep 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjhsf commented Sep 15, 2025 • edited by jhefferman-sfdc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjhsf commented Sep 15, 2025

Uh oh!

wjhsf commented Sep 15, 2025

Uh oh!

wjhsf left a comment

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!

jhefferman-sfdc Sep 16, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

jhefferman-sfdc commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhefferman-sfdc commented Sep 16, 2025

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jhefferman-sfdc commented Sep 12, 2025 •

edited

Loading

jhefferman-sfdc commented Sep 13, 2025 •

edited

Loading

wjhsf commented Sep 15, 2025 •

edited by jhefferman-sfdc

Loading

jhefferman-sfdc Sep 16, 2025 •

edited

Loading

jhefferman-sfdc commented Sep 16, 2025 •

edited

Loading