fix: context property observation, tests (#5536)

jhefferman-sfdc · web-flow · commit 4ca7f2abaf94 · 2025-10-13T21:44:39.000Z
* fix: context property observation, tests

* fix: exception for SSRv1

* fix: pr name

* fix: un-revert change

* chore: expand bug explanation

* chore: expand bug explanation

* chore: expand bug explanation

* fix: gate and related tests

* fix: bundlesize
diff --git a/packages/@lwc/engine-core/src/framework/modules/context.ts b/packages/@lwc/engine-core/src/framework/modules/context.ts
@@ -6,14 +6,14 @@
  */
 import {
     isUndefined,
-    getPrototypeOf,
     keys,
     getContextKeys,
     ArrayFilter,
     ContextEventName,
     isTrustedContext,
     type ContextProvidedCallback,
     type ContextBinding as IContextBinding,
+    getPrototypeOf,
 } from '@lwc/shared';
 import { type VM } from '../vm';
 import { logWarnOnce } from '../../shared/logger';
@@ -81,6 +81,38 @@ class ContextBinding<C extends object> implements IContextBinding<C> {
 }
 
 export function connectContext(vm: VM) {
+    /**
+     * If ENABLE_LEGACY_CONTEXT_CONNECTION is true, enumerates directly on the component
+     * which can result in the component lifecycle observing properties that are not typically observed.
+     * See PR #5536 for more information.
+     */
+    if (lwcRuntimeFlags.ENABLE_LEGACY_CONTEXT_CONNECTION) {
+        connect(vm, keys(getPrototypeOf(vm.component)), vm.component);
+    } else {
+        // Non-decorated objects
+        connect(vm, keys(vm.cmpFields), vm.cmpFields);
+        // Decorated objects like @api context
+        connect(vm, keys(vm.cmpProps), vm.cmpProps);
+    }
+}
+
+export function disconnectContext(vm: VM) {
+    /**
+     * If ENABLE_LEGACY_CONTEXT_CONNECTION is true, enumerates directly on the component
+     * which can result in the component lifecycle observing properties that are not typically observed.
+     * See PR #5536 for more information.
+     */
+    if (lwcRuntimeFlags.ENABLE_LEGACY_CONTEXT_CONNECTION) {
+        connect(vm, keys(getPrototypeOf(vm.component)), vm.component);
+    } else {
+        // Non-decorated objects
+        disconnect(vm, keys(vm.cmpFields), vm.cmpFields);
+        // Decorated objects like @api context
+        disconnect(vm, keys(vm.cmpProps), vm.cmpProps);
+    }
+}
+
+function connect(vm: VM, enumerableKeys: string[], contextContainer: any) {
     const contextKeys = getContextKeys();
 
     if (isUndefined(contextKeys)) {
@@ -90,9 +122,8 @@ export function connectContext(vm: VM) {
     const { connectContext } = contextKeys;
     const { component } = vm;
 
-    const enumerableKeys = keys(getPrototypeOf(component));
     const contextfulKeys = ArrayFilter.call(enumerableKeys, (enumerableKey) =>
-        isTrustedContext((component as any)[enumerableKey])
+        isTrustedContext(contextContainer[enumerableKey])
     );
 
     if (contextfulKeys.length === 0) {
@@ -103,7 +134,7 @@ export function connectContext(vm: VM) {
 
     try {
         for (let i = 0; i < contextfulKeys.length; i++) {
-            (component as any)[contextfulKeys[i]][connectContext](
+            contextContainer[contextfulKeys[i]][connectContext](
                 new ContextBinding(vm, component, providedContextVarieties)
             );
         }
@@ -116,7 +147,7 @@ export function connectContext(vm: VM) {
     }
 }
 
-export function disconnectContext(vm: VM) {
+function disconnect(vm: VM, enumerableKeys: string[], contextContainer: any) {
     const contextKeys = getContextKeys();
 
     if (!contextKeys) {
@@ -126,9 +157,8 @@ export function disconnectContext(vm: VM) {
     const { disconnectContext } = contextKeys;
     const { component } = vm;
 
-    const enumerableKeys = keys(getPrototypeOf(component));
     const contextfulKeys = ArrayFilter.call(enumerableKeys, (enumerableKey) =>
-        isTrustedContext((component as any)[enumerableKey])
+        isTrustedContext(contextContainer[enumerableKey])
     );
 
     if (contextfulKeys.length === 0) {
@@ -137,7 +167,7 @@ export function disconnectContext(vm: VM) {
 
     try {
         for (let i = 0; i < contextfulKeys.length; i++) {
-            (component as any)[contextfulKeys[i]][disconnectContext](component);
+            contextContainer[contextfulKeys[i]][disconnectContext](component);
         }
     } catch (err: any) {
         logWarnOnce(
diff --git a/packages/@lwc/features/src/index.ts b/packages/@lwc/features/src/index.ts
@@ -25,6 +25,7 @@ const features: FeatureFlagMap = {
     LEGACY_LOCKER_ENABLED: null,
     DISABLE_LEGACY_VALIDATION: null,
     DISABLE_DETACHED_REHYDRATION: null,
+    ENABLE_LEGACY_CONTEXT_CONNECTION: null,
 };
 
 if (!(globalThis as any).lwcRuntimeFlags) {
diff --git a/packages/@lwc/features/src/types.ts b/packages/@lwc/features/src/types.ts
@@ -106,6 +106,13 @@ export interface FeatureFlagMap {
      * Applies to rehydration performed while flushing the rehydration queue.
      */
     DISABLE_DETACHED_REHYDRATION: FeatureFlagValue;
+
+    /**
+     * If true, enables legacy context connection and disconnection which can result in the component lifecycle
+     * observing properties that are not typically observed. ENABLE_EXPERIMENTAL_SIGNALS must also be enabled for
+     * this flag to have an effect. See PR #5536 for more information.
+     */
+    ENABLE_LEGACY_CONTEXT_CONNECTION: FeatureFlagValue;
 }
 
 export type FeatureFlagName = keyof FeatureFlagMap;
diff --git a/packages/@lwc/integration-not-karma/helpers/context.js b/packages/@lwc/integration-not-karma/helpers/context.js
@@ -0,0 +1,22 @@
+import {
+    setContextKeys,
+    setTrustedContextSet,
+    __dangerous_do_not_use_addTrustedContext,
+} from 'lwc';
+
+export const connectContext = Symbol.for('connectContext');
+export const disconnectContext = Symbol.for('disconnectContext');
+export const trustedContext = new WeakSet();
+
+export function initContext() {
+    try {
+        setTrustedContextSet(trustedContext);
+        setContextKeys({ connectContext, disconnectContext });
+    } catch {
+        // Context already initialized, ignore
+    }
+}
+
+export function addTrustedContext(context) {
+    __dangerous_do_not_use_addTrustedContext(context);
+}
diff --git a/packages/@lwc/integration-not-karma/helpers/setup.js b/packages/@lwc/integration-not-karma/helpers/setup.js
@@ -1,10 +1,16 @@
 // This import ensures that the global `Mocha` object is present for mutation.
 import { JestAsymmetricMatchers, JestChaiExpect, JestExtend } from '@vitest/expect';
 import * as chai from 'chai';
+import { setFeatureFlagForTest } from 'lwc';
 import { registerCustomMatchers } from './matchers/index.js';
 import { initSignals } from './signals.js';
+import { initContext } from './context.js';
 
 initSignals();
+initContext();
+
+// Enabling signals by default to increase coverage
+setFeatureFlagForTest('ENABLE_EXPERIMENTAL_SIGNALS', true);
 
 // allows using expect.extend instead of chai.use to extend plugins
 chai.use(JestExtend);
diff --git a/packages/@lwc/integration-not-karma/test-hydration/context/index.spec.js b/packages/@lwc/integration-not-karma/test-hydration/context/index.spec.js
@@ -39,16 +39,19 @@ export default {
         // Assert context is disconnected when components are removed
         assertContextDisconnected(target, snapshot);
 
-        // Expect an error as one context was generated twice.
-        // Expect an error as one context was malformed (did not define connectContext or disconnectContext methods).
-        // Expect server/client context output parity (no hydration warnings)
-        expectConsoleCalls(consoleCalls, {
-            error: [],
-            warn: [
-                'Attempted to connect to trusted context but received the following error',
-                'Multiple contexts of the same variety were provided. Only the first context will be used.',
-            ],
-        });
+        // Legacy SSRv1 does not support context when inherited
+        if (!process.env.ENGINE_SERVER) {
+            // Expect an error as one context was generated twice.
+            // Expect an error as one context was malformed (did not define connectContext or disconnectContext methods).
+            // Expect server/client context output parity (no hydration warnings)
+            expectConsoleCalls(consoleCalls, {
+                error: [],
+                warn: [
+                    'Attempted to connect to trusted context but received the following error',
+                    'Multiple contexts of the same variety were provided. Only the first context will be used.',
+                ],
+            });
+        }
     },
 };
 
diff --git a/packages/@lwc/integration-not-karma/test-hydration/context/x/base/base.js b/packages/@lwc/integration-not-karma/test-hydration/context/x/base/base.js
@@ -2,6 +2,24 @@ import { LightningElement, api } from 'lwc';
 
 export default class Base extends LightningElement {
     @api disconnect;
+    // Tests that non-decorated and inherited context works correctly
+    _context;
+    // Tests that decorated inherited context works correctly
+    @api anotherContext;
+
+    constructor(context, anotherContext) {
+        super();
+        this.context = context;
+        this.anotherContext = anotherContext;
+    }
+
+    @api set context(value) {
+        this._context = value;
+    }
+
+    get context() {
+        return this._context;
+    }
 
     disconnectedCallback() {
         if (this.disconnect) {
diff --git a/packages/@lwc/integration-not-karma/test-hydration/context/x/child/child.js b/packages/@lwc/integration-not-karma/test-hydration/context/x/child/child.js
@@ -4,6 +4,9 @@ import { defineContext } from 'x/contextManager';
 import { parentContextFactory, anotherParentContextFactory } from 'x/parentContext';
 
 export default class Child extends Base {
-    @api context = defineContext(parentContextFactory)();
     @api anotherContext = defineContext(anotherParentContextFactory)();
+
+    constructor() {
+        super(defineContext(parentContextFactory)());
+    }
 }
diff --git a/packages/@lwc/integration-not-karma/test-hydration/context/x/contextManager/contextManager.js b/packages/@lwc/integration-not-karma/test-hydration/context/x/contextManager/contextManager.js
@@ -1,15 +1,11 @@
 import {
-    setContextKeys,
-    setTrustedContextSet,
-    __dangerous_do_not_use_addTrustedContext,
-} from 'lwc';
+    connectContext,
+    disconnectContext,
+    addTrustedContext,
+    initContext,
+} from '../../../../helpers/context';
 
-const connectContext = Symbol('connectContext');
-const disconnectContext = Symbol('disconnectContext');
-const trustedContext = new WeakSet();
-
-setTrustedContextSet(trustedContext);
-setContextKeys({ connectContext, disconnectContext });
+initContext();
 
 class MockContextSignal {
     connectProvidedComponent;
@@ -20,7 +16,7 @@ class MockContextSignal {
         this.value = initialValue;
         this.contextDefinition = contextDefinition;
         this.fromContext = fromContext;
-        __dangerous_do_not_use_addTrustedContext(this);
+        addTrustedContext(this);
     }
     [connectContext](runtimeAdapter) {
         this.connectProvidedComponent = runtimeAdapter.component;
@@ -42,7 +38,7 @@ class MockContextSignal {
 // This is a malformed context signal that does not implement the connectContext or disconnectContext methods
 class MockMalformedContextSignal {
     constructor() {
-        trustedContext.add(this);
+        addTrustedContext(this);
     }
 }
 
diff --git a/packages/@lwc/integration-not-karma/test-hydration/context/x/grandparent/grandparent.js b/packages/@lwc/integration-not-karma/test-hydration/context/x/grandparent/grandparent.js
@@ -3,6 +3,9 @@ import Base from 'x/base';
 import { grandparentContextFactory, anotherGrandparentContextFactory } from 'x/grandparentContext';
 
 export default class Grandparent extends Base {
-    @api context = grandparentContextFactory('grandparent provided value');
     @api anotherContext = anotherGrandparentContextFactory('another grandparent provided value');
+
+    constructor() {
+        super(grandparentContextFactory('grandparent provided value'));
+    }
 }
diff --git a/packages/@lwc/integration-not-karma/test-hydration/context/x/parent/parent.js b/packages/@lwc/integration-not-karma/test-hydration/context/x/parent/parent.js
@@ -1,8 +1,8 @@
-import { api } from 'lwc';
 import { parentContextFactory, anotherParentContextFactory } from 'x/parentContext';
 import Base from 'x/base';
 
 export default class Parent extends Base {
-    @api context = parentContextFactory();
-    @api anotherContext = anotherParentContextFactory();
+    constructor() {
+        super(parentContextFactory(), anotherParentContextFactory());
+    }
 }
diff --git a/packages/@lwc/integration-not-karma/test/reactivity/delayed-render/index.spec.js b/packages/@lwc/integration-not-karma/test/reactivity/delayed-render/index.spec.js
@@ -0,0 +1,102 @@
+import { createElement, setFeatureFlagForTest } from 'lwc';
+
+import Test from 'x/test';
+
+// A bug (W-19830319) has shown that if child component properties are incorrectly marked for observation by the lifecycle, they can trigger the parent component to re-render
+// and this will cause undesired effects. This bug manifested due to context connection/disconnection erroneously accessing properties, but it could theoretically occur in any instance where
+// the "live" component property is accessed instead of via vm.cmpFields / vm.cmpProps.
+describe('Unobserved properties should trigger additional re-renders', () => {
+    it('when signals is enabled and legacy context connection is enabled', async () => {
+        // This is on by default, but enabled here to confirm the case
+        setFeatureFlagForTest('ENABLE_EXPERIMENTAL_SIGNALS', true);
+        setFeatureFlagForTest('ENABLE_LEGACY_CONTEXT_CONNECTION', true);
+
+        const elm = createElement('x-test', { is: Test });
+        document.body.appendChild(elm);
+        // Specific sequence (it is rather nuanced and requires 1. parent to switch templates and 2. child to mutate a property in a setTimeout function or similar):
+        // 1. The template change results in the parent component being marked as dirty.
+        // 1a. Marking the parent as dirty sets the currentReactiveObserver to the parent, here: https://github.com/salesforce/lwc/blob/master/packages/%40lwc/engine-core/src/libs/mutation-tracker/index.ts#L83
+        // 2. The new template doesn't contain the child so disconnectContext is called on the child component. The BUG: If the child properties are incorrectly observed then riggering disconnectContext marks all child properties
+        // for observation using the currentReactiveObserver of the parent set in 1a. here: https://github.com/salesforce/lwc/blob/master/packages/%40lwc/engine-core/src/libs/mutation-tracker/index.ts#L60
+        // 3. Next, a delayed property mutation inside the child component's renderedCallback occurs and this delayed (post disconnection) mutation triggers valueMutated, where all the parent properties are registered listeners.
+        // That happens here: https://github.com/salesforce/lwc/blob/master/packages/%40lwc/engine-core/src/libs/mutation-tracker/index.ts#L41
+        // 3b. This causes the parent component to re-render.
+        elm.switchToEmptyTemplate();
+
+        // Before the bugfix (W-19830319/PR-5536), expecting 2 renders when signals was enabled would fail.
+        // An erroneous 3rd render would occur when signals was enabled due to the behavior described above.
+        setTimeout(() => {
+            expect(elm.renderCount).toBe(3); // because ENABLE_LEGACY_SIGNAL_CONTEXT_VALIDATION = true this will render 3 times (incorrect)
+        });
+    });
+
+    it('when signals is enabled, legacy context connection is true and detached rehydration is enabled', async () => {
+        setFeatureFlagForTest('ENABLE_EXPERIMENTAL_SIGNALS', true);
+        setFeatureFlagForTest('ENABLE_LEGACY_CONTEXT_CONNECTION', true);
+        setFeatureFlagForTest('DISABLE_DETACHED_REHYDRATION', true);
+
+        const elm = createElement('x-test', { is: Test });
+        document.body.appendChild(elm);
+        elm.switchToEmptyTemplate();
+
+        // Enabling detached rehydration does not change the behavior (bug still exists and the third render occurs)
+        setTimeout(() => {
+            expect(elm.renderCount).toBe(3);
+        });
+    });
+
+    afterAll(() => {
+        setFeatureFlagForTest('ENABLE_EXPERIMENTAL_SIGNALS', true);
+        setFeatureFlagForTest('ENABLE_LEGACY_CONTEXT_CONNECTION', false);
+        setFeatureFlagForTest('DISABLE_DETACHED_REHYDRATION', false);
+    });
+});
+
+describe('Unobserved properties should NOT trigger additional re-renders', () => {
+    it('when signals is enabled, legacy context connection is disabled and detached rehydration is enabled', async () => {
+        setFeatureFlagForTest('ENABLE_EXPERIMENTAL_SIGNALS', true);
+        setFeatureFlagForTest('ENABLE_LEGACY_CONTEXT_CONNECTION', false);
+        setFeatureFlagForTest('DISABLE_DETACHED_REHYDRATION', true);
+
+        const elm = createElement('x-test', { is: Test });
+        document.body.appendChild(elm);
+        elm.switchToEmptyTemplate();
+
+        // Enabling detached rehydration does not change the behavior
+        setTimeout(() => {
+            expect(elm.renderCount).toBe(2);
+        });
+    });
+
+    it('when signals is enabled and legacy context connection is disabled', async () => {
+        setFeatureFlagForTest('ENABLE_EXPERIMENTAL_SIGNALS', true);
+        setFeatureFlagForTest('ENABLE_LEGACY_CONTEXT_CONNECTION', false);
+
+        const elm = createElement('x-test', { is: Test });
+        document.body.appendChild(elm);
+        elm.switchToEmptyTemplate();
+
+        // CORRECT: This component should render twice as the corrected context connection is in use.
+        setTimeout(() => {
+            expect(elm.renderCount).toBe(2);
+        });
+    });
+
+    it('when signals is disabled', async () => {
+        setFeatureFlagForTest('ENABLE_EXPERIMENTAL_SIGNALS', false);
+
+        const elm = createElement('x-test', { is: Test });
+        document.body.appendChild(elm);
+        elm.switchToEmptyTemplate();
+
+        // CORRECT: This component should render twice as signals is disabled and there should be no observation bugs elsewhere in the lifecycle.
+        setTimeout(() => {
+            expect(elm.renderCount).toBe(2);
+        });
+    });
+
+    afterAll(() => {
+        setFeatureFlagForTest('ENABLE_EXPERIMENTAL_SIGNALS', true);
+        setFeatureFlagForTest('DISABLE_DETACHED_REHYDRATION', false);
+    });
+});
diff --git a/packages/@lwc/integration-not-karma/test/reactivity/delayed-render/x/child/child.js b/packages/@lwc/integration-not-karma/test/reactivity/delayed-render/x/child/child.js
diff --git a/packages/@lwc/integration-not-karma/test/reactivity/delayed-render/x/test/empty.html b/packages/@lwc/integration-not-karma/test/reactivity/delayed-render/x/test/empty.html
diff --git a/packages/@lwc/integration-not-karma/test/reactivity/delayed-render/x/test/test.js b/packages/@lwc/integration-not-karma/test/reactivity/delayed-render/x/test/test.js
diff --git a/packages/@lwc/integration-not-karma/test/reactivity/delayed-render/x/test/withChild.html b/packages/@lwc/integration-not-karma/test/reactivity/delayed-render/x/test/withChild.html
diff --git a/scripts/bundlesize/bundlesize.config.json b/scripts/bundlesize/bundlesize.config.json
