-
Notifications
You must be signed in to change notification settings - Fork 439
isTrustedSignal/Context should return false when no trusted set has been defined #5492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
26e22d0
5973cc7
f862a2b
0e8cdd4
0804c69
ed9c66a
8be0a75
550da05
2734268
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| /* | ||
| * Copyright (c) 2025, 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 { describe, it, expect, vi, beforeAll, afterEach, afterAll } from 'vitest'; | ||
| import { setTrustedContextSet, setContextKeys } from '@lwc/shared'; | ||
| import { logWarnOnce } from '../../shared/logger'; | ||
| import { connectContext, disconnectContext } from '../modules/context'; | ||
|
|
||
| // Mock the logger to inspect console output during tests | ||
| vi.mock('../../shared/logger', () => ({ | ||
| logWarnOnce: vi.fn(), | ||
| })); | ||
|
|
||
| // Create mock component with a regular, non-contextful property | ||
| const mockComponent = {}; | ||
| Object.setPrototypeOf(mockComponent, { | ||
| regularProp: 'not contextful', | ||
| }); | ||
|
|
||
| // Create mock renderer | ||
| const mockRenderer = { | ||
| registerContextProvider: vi.fn(), | ||
| }; | ||
|
|
||
| // Create mock VM | ||
| const mockVM = { | ||
| component: mockComponent, | ||
| elm: null, | ||
| renderer: mockRenderer, | ||
| } as any; | ||
|
|
||
| if (!(globalThis as any).lwcRuntimeFlags) { | ||
| Object.defineProperty(globalThis, 'lwcRuntimeFlags', { value: {} }); | ||
| } | ||
|
|
||
| /** | ||
| * Need to be able to set and reset the flags at will (lwc/features doesn't provide this) | ||
| */ | ||
| const setFeatureFlag = (name: string, value: boolean) => { | ||
| (globalThis as any).lwcRuntimeFlags[name] = value; | ||
| }; | ||
|
Comment on lines
+42
to
+44
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can re-use the setFeatureFlagsForTest helper instead of defining your own. |
||
|
|
||
| /** | ||
| * These tests test that properties are correctly validated within the connectContext and disconnectContext | ||
| * functions regardless of whether trusted context has been defined or not. | ||
| * Integration tests have been used for extensive coverage of the LWC context feature, but this particular | ||
| * scenario is best isolated and unit tested as it involves manipulation of the trusted context API. | ||
| * See bug fix: #5492 | ||
| */ | ||
| describe('context functions', () => { | ||
| beforeAll(() => { | ||
| const connectContext = Symbol('connectContext'); | ||
| const disconnectContext = Symbol('disconnectContext'); | ||
| setContextKeys({ connectContext, disconnectContext }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| setFeatureFlag('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', false); | ||
| }); | ||
|
|
||
| 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' | ||
| ); | ||
| }); | ||
|
Comment on lines
+68
to
+75
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
|
|
||
| it('should not log a warning when trustedContext is not defined and connectContext is called with non-legacy context validation', () => { | ||
| setFeatureFlag('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', false); | ||
| connectContext(mockVM); | ||
| expect(logWarnOnce).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should log a warning when trustedContext is not defined and disconnectContext is called with legacy signal context validation', () => { | ||
| setFeatureFlag('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', true); | ||
| disconnectContext(mockVM); | ||
| expect(logWarnOnce).toHaveBeenCalledWith( | ||
| 'Attempted to disconnect from trusted context but received the following error: component[contextfulKeys[i]][disconnectContext2] is not a function' | ||
| ); | ||
| }); | ||
|
|
||
| it('should not log a warning when trustedContext is not defined and disconnectContext is called with non-legacy context validation', () => { | ||
| setFeatureFlag('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', false); | ||
| disconnectContext(mockVM); | ||
| expect(logWarnOnce).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('with trusted context set', () => { | ||
| it('should not log warnings when trustedContext is defined', () => { | ||
| setTrustedContextSet(new WeakSet()); | ||
| setFeatureFlag('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', true); | ||
| connectContext(mockVM); | ||
| disconnectContext(mockVM); | ||
| setFeatureFlag('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', false); | ||
| connectContext(mockVM); | ||
| disconnectContext(mockVM); | ||
| expect(logWarnOnce).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| /* | ||
| * Copyright (c) 2019, 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 { describe, it, expect, vi, afterEach, beforeEach, afterAll, beforeAll } from 'vitest'; | ||
| import { setTrustedSignalSet } from '@lwc/shared'; | ||
| import { componentValueObserved } from '../../framework/mutation-tracker'; | ||
|
|
||
| // Create a mock VM object with required properties | ||
| const mockVM = { | ||
| component: {}, | ||
| tro: { | ||
| isObserving: () => true, | ||
| }, | ||
| } as any; | ||
|
|
||
| if (!(globalThis as any).lwcRuntimeFlags) { | ||
| Object.defineProperty(globalThis, 'lwcRuntimeFlags', { value: {} }); | ||
| } | ||
|
|
||
| /** | ||
| * Need to be able to set and reset the flags at will (lwc/features doesn't provide this) | ||
| */ | ||
| const setFeatureFlag = (name: string, value: boolean) => { | ||
| (globalThis as any).lwcRuntimeFlags[name] = value; | ||
| }; | ||
|
|
||
| /** | ||
| * These tests check that properties are correctly validated within the mutation-tracker | ||
| * regardless of whether trusted context has been defined by a state manager or not. | ||
| * Integration tests have been used for extensive coverage of the LWC signals feature, but this particular | ||
| * scenario is best isolated and unit tested as it involves manipulation of the trusted context API. | ||
| * See bug fix: #5492 | ||
| */ | ||
| describe('mutation-tracker', () => { | ||
| describe('trustedSignal set not defined', () => { | ||
| it('should not throw when componentValueObserved is called using the new signals validation', () => { | ||
| setFeatureFlag('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', false); | ||
| expect(() => { | ||
| componentValueObserved(mockVM, 'testKey', {}); | ||
| }).not.toThrow(); | ||
| }); | ||
|
|
||
| it('should throw when componentValueObserved is called using legacy signals validation', () => { | ||
| setFeatureFlag('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', true); | ||
| expect(() => { | ||
| componentValueObserved(mockVM, 'testKey', {}); | ||
| }).toThrow(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('trustedSignal set defined', () => { | ||
| it('should not throw when componentValueObserved is called, regardless of validation type', () => { | ||
| setTrustedSignalSet(new WeakSet()); | ||
| setFeatureFlag('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', false); | ||
| expect(() => { | ||
| componentValueObserved(mockVM, 'testKey', {}); | ||
| }).not.toThrow(); | ||
|
|
||
| setFeatureFlag('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', true); | ||
| expect(() => { | ||
| componentValueObserved(mockVM, 'testKey', {}); | ||
| }).not.toThrow(); | ||
| }); | ||
| }); | ||
|
|
||
| beforeAll(() => { | ||
| setFeatureFlag('ENABLE_EXPERIMENTAL_SIGNALS', true); | ||
| setFeatureFlag('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', true); | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| setFeatureFlag('ENABLE_EXPERIMENTAL_SIGNALS', false); | ||
| setFeatureFlag('ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION', false); | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| vi.stubEnv('IS_BROWSER', 'true'); // Signals is a browser-only feature | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.unstubAllEnvs(); // Reset environment variables after each test | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import { | |
| ArrayFilter, | ||
| ContextEventName, | ||
| isTrustedContext, | ||
| legacyIsTrustedContext, | ||
| type ContextProvidedCallback, | ||
| type ContextBinding as IContextBinding, | ||
| } from '@lwc/shared'; | ||
|
|
@@ -92,7 +93,17 @@ export function connectContext(vm: VM) { | |
|
|
||
| const enumerableKeys = keys(getPrototypeOf(component)); | ||
| const contextfulKeys = ArrayFilter.call(enumerableKeys, (enumerableKey) => | ||
| isTrustedContext((component as any)[enumerableKey]) | ||
| /** | ||
| * 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 object values were | ||
| * being considered context in environments where the trustedContext | ||
| * set had not been provided. The runtime flag has been added as a killswitch | ||
| * in case the fix needs to be reverted. | ||
| */ | ||
| lwcRuntimeFlags.ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION | ||
| ? legacyIsTrustedContext((component as any)[enumerableKey]) | ||
| : isTrustedContext((component as any)[enumerableKey]) | ||
| ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jhefferman-sfdc - Just to confirm, this is how it will work with the killswitch right? killswitch on => killswitch off =>
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jmsjtu killswitch on => ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION is on => use current legacy validation which were are fixing ( killswitch off => ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION is off => use corrected validation ( Is that OK / makes sense?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 |
||
|
|
||
| if (contextfulKeys.length === 0) { | ||
|
|
@@ -128,7 +139,17 @@ export function disconnectContext(vm: VM) { | |
|
|
||
| const enumerableKeys = keys(getPrototypeOf(component)); | ||
| const contextfulKeys = ArrayFilter.call(enumerableKeys, (enumerableKey) => | ||
| isTrustedContext((component as any)[enumerableKey]) | ||
| /** | ||
| * 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 object values were | ||
| * being considered context in environments where the trustedContext | ||
| * set had not been provided. The runtime flag has been added as a killswitch | ||
| * in case the fix needs to be reverted. | ||
| */ | ||
| lwcRuntimeFlags.ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION | ||
| ? legacyIsTrustedContext((component as any)[enumerableKey]) | ||
| : isTrustedContext((component as any)[enumerableKey]) | ||
| ); | ||
|
|
||
| if (contextfulKeys.length === 0) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ describe('context', () => { | |
| let setTrustedContextSet: (signals: WeakSet<object>) => void; | ||
| let addTrustedContext: (signal: object) => void; | ||
| let isTrustedContext: (target: object) => boolean; | ||
| let legacyIsTrustedContext: (target: object) => boolean; | ||
|
|
||
| beforeEach(async () => { | ||
| vi.resetModules(); | ||
|
|
@@ -21,6 +22,7 @@ describe('context', () => { | |
| setTrustedContextSet = contextModule.setTrustedContextSet; | ||
| addTrustedContext = contextModule.addTrustedContext; | ||
| isTrustedContext = contextModule.isTrustedContext; | ||
| legacyIsTrustedContext = contextModule.legacyIsTrustedContext; | ||
| }); | ||
|
|
||
| it('should set and get context keys', () => { | ||
|
|
@@ -96,8 +98,12 @@ describe('context', () => { | |
| expect(isTrustedContext({})).toBe(false); | ||
| }); | ||
|
|
||
| it('should return true for all calls when trustedContexts is not set', () => { | ||
| expect(isTrustedContext({})).toBe(true); | ||
| 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); | ||
|
Comment on lines
+101
to
+106
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, I've added some tests around this change. |
||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have some helper methods defined already for the loggers you could use.