-
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 4 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 | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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'; | ||||||||||||||
|
|
@@ -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 component properties 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)) || | ||||||||||||||
| (!lwcRuntimeFlags.ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION && isTrustedSignal(target)) | ||||||||||||||
|
||||||||||||||
| (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
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.
I had a slight preference for the verbose syntax but on second thoughts, you're right
| 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. |
||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,15 +47,30 @@ export function setTrustedContextSet(context: WeakSet<object>) { | |
| } | ||
|
|
||
| export function addTrustedContext(contextParticipant: object) { | ||
| // This should be a no-op when the trustedSignals set isn't set by runtime | ||
| // This should be a no-op when the trustedContext set isn't set by runtime | ||
| trustedContext?.add(contextParticipant); | ||
| } | ||
|
|
||
| export function isTrustedContext(target: object): boolean { | ||
| /** | ||
| * 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 | ||
|
||
| * 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. | ||
| */ | ||
| export function legacyIsTrustedContext(target: object): boolean { | ||
| if (!trustedContext) { | ||
| // The runtime didn't set a trustedContext set | ||
| // this check should only be performed for runtimes that care about filtering context participants to track | ||
| return true; | ||
| } | ||
| return trustedContext.has(target); | ||
| } | ||
|
|
||
| export function isTrustedContext(target: object): boolean { | ||
| if (!trustedContext) { | ||
| return false; | ||
| } | ||
| return trustedContext.has(target); | ||
| } | ||
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.
@jhefferman-sfdc - Just to confirm, this is how it will work with the killswitch right?
killswitch on =>
ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATIONis offkillswitch off =>
ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATIONis onThere 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.
@jmsjtu
ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATIONis 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?
Uh oh!
There was an error while loading. Please reload this page.
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.
@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