From b3fba42a1cfb0a130d034831824220abacd01698 Mon Sep 17 00:00:00 2001 From: sanal1729 Date: Sun, 14 Jun 2026 15:50:48 +0100 Subject: [PATCH] bug fix --- .../angular/auto-instrumentation.spec.ts | 131 ++++++++++++++++++ .../angular/auto-instrumentation.ts | 87 +++++++++--- 2 files changed, 195 insertions(+), 23 deletions(-) create mode 100644 packages/angular-render-scan/src/infrastructure/angular/auto-instrumentation.spec.ts diff --git a/packages/angular-render-scan/src/infrastructure/angular/auto-instrumentation.spec.ts b/packages/angular-render-scan/src/infrastructure/angular/auto-instrumentation.spec.ts new file mode 100644 index 0000000..b8bb55b --- /dev/null +++ b/packages/angular-render-scan/src/infrastructure/angular/auto-instrumentation.spec.ts @@ -0,0 +1,131 @@ +import { describe, expect, it } from 'vitest'; +import { patchSignalsOnInstance } from './auto-instrumentation'; + +// A minimal stand-in for an Angular signal: a callable with a [SIGNAL] symbol +// node plus set/update/asReadonly, matching what the instrumentation detects. +const SIGNAL = Symbol('SIGNAL'); +function makeSignal(initial: T) { + const node = { value: initial, equal: (a: T, b: T) => a === b, debugName: undefined }; + const fn: any = function () { + return node.value; + }; + fn[SIGNAL] = node; + fn.set = (v: T) => { + node.value = v; + }; + fn.update = (f: (v: T) => T) => { + node.value = f(node.value); + }; + fn.asReadonly = () => fn; + return fn; +} + +// Stand-ins that mirror the real framework shapes from the bug reports: a +// reactive-forms control tree and a router URL segment tree, both of which keep +// their children in plain-object dictionaries that the framework walks with +// Object.keys(). The original instrumentation injected an enumerable marker into +// those dictionaries, so iteration picked up a `true` and called methods on it. +class AbstractControl { + _updateTreeValidity(): void { + /* no-op */ + } +} +class FormControl extends AbstractControl {} +class FormGroup extends AbstractControl { + controls: Record; + constructor() { + super(); + this.controls = { name: new FormControl(), email: new FormControl() }; + } + _forEachChild(cb: (c: AbstractControl) => void): void { + Object.keys(this.controls).forEach(k => cb(this.controls[k])); + } + override _updateTreeValidity(): void { + this._forEachChild(ctrl => ctrl._updateTreeValidity()); + } +} + +class UrlSegmentGroup { + children: Record = {}; + hasChildren(): boolean { + return Object.keys(this.children).length > 0; + } +} + +describe('patchSignalsOnInstance', () => { + it('does not corrupt reactive-forms control trees', () => { + const form = new FormGroup(); + const component = { name: 'FormComponent', form }; + + patchSignalsOnInstance(component, 'FormComponent'); + + // The controls dictionary must not gain an injected enumerable key... + expect(Object.keys(form.controls)).toEqual(['name', 'email']); + // ...so framework iteration over it stays valid (this threw the original + // "ctrl._updateTreeValidity is not a function"). + expect(() => form._updateTreeValidity()).not.toThrow(); + }); + + it('does not corrupt router URL segment trees', () => { + const root = new UrlSegmentGroup(); + root.children = { primary: new UrlSegmentGroup() }; + const component = { name: 'NavComponent', tree: root }; + + patchSignalsOnInstance(component, 'NavComponent'); + + expect(Object.keys(root.children)).toEqual(['primary']); + // The original bug surfaced as "segment.hasChildren is not a function" when + // serializing children — emulate that walk. + expect(() => + Object.keys(root.children).forEach(k => root.children[k].hasChildren()) + ).not.toThrow(); + }); + + it('never writes enumerable markers onto any walked object', () => { + class ViewModel { + nested = { deep: { leaf: 1 } }; + } + const component = { name: 'C', vm: new ViewModel() }; + + patchSignalsOnInstance(component, 'C'); + + const polluted = (obj: object): boolean => + Object.keys(obj).some(k => k.includes('angularRenderScan')); + expect(polluted(component)).toBe(false); + expect(polluted(component.vm)).toBe(false); + expect(polluted(component.vm.nested)).toBe(false); + }); + + it('wraps real signals but leaves plain methods untouched', () => { + const realSignal = makeSignal(1); + const plainMethod = function () { + return 'hello'; + }; + const component: any = { + name: 'C', + count: realSignal, + doThing: plainMethod + }; + + patchSignalsOnInstance(component, 'C'); + + // Real signal is wrapped (new function) but still reads through and is settable. + expect(component.count).not.toBe(realSignal); + expect(component.count()).toBe(1); + component.count.set(5); + expect(component.count()).toBe(5); + + // The cache-bug regression: a plain function property next to a signal must + // NOT be wrapped as a fake signal — identity is preserved. + expect(component.doThing).toBe(plainMethod); + }); + + it('is safe against cyclic object graphs', () => { + const a: any = { name: 'A' }; + const b: any = { name: 'B', a }; + a.b = b; + const component = { name: 'C', a }; + + expect(() => patchSignalsOnInstance(component, 'C')).not.toThrow(); + }); +}); diff --git a/packages/angular-render-scan/src/infrastructure/angular/auto-instrumentation.ts b/packages/angular-render-scan/src/infrastructure/angular/auto-instrumentation.ts index cb9df27..dbea6ca 100644 --- a/packages/angular-render-scan/src/infrastructure/angular/auto-instrumentation.ts +++ b/packages/angular-render-scan/src/infrastructure/angular/auto-instrumentation.ts @@ -16,19 +16,53 @@ import type { AngularRenderChangedInput, AngularRenderScanRenderDetails } from ' const ProfilerEventTemplateUpdateStart = 2; const ProfilerEventTemplateUpdateEnd = 3; -function patchSignalsOnInstance(instance: any, compName: string): void { +// How deep into a component's object graph we walk looking for nested signals. +const MAX_SIGNAL_PATCH_DEPTH = 3; + +// Constructor names we must never recurse into. Walking these mutates / wraps +// Angular framework internals and corrupts them — e.g. reactive-forms controls +// and router URL trees, whose children are plain-object containers that Angular +// iterates with Object.keys(). See exclusions in the recursion branch below. +const FRAMEWORK_CTOR_EXCLUSIONS = [ + 'ElementRef', + 'ViewContainerRef', + 'ChangeDetectorRef', + 'NgZone', + 'ApplicationRef', + 'Router', + 'ActivatedRoute', + 'UrlTree', + 'UrlSegment', + 'UrlSegmentGroup', + 'AbstractControl', + 'FormControl', + 'FormGroup', + 'FormArray', + 'FormRecord', + 'NgControl', + 'EventEmitter' +]; + +// Per-prop scan for the signal symbol. Must NOT be cached across props: a cached +// symbol would make us treat any later function-valued property as a signal. +function getSignalSymbol(obj: any): symbol | undefined { + if (!obj || (typeof obj !== 'object' && typeof obj !== 'function')) return undefined; + const sym = Reflect.ownKeys(obj).find(k => typeof k === 'symbol' && String(k).includes('SIGNAL')); + return sym as symbol | undefined; +} + +export function patchSignalsOnInstance( + instance: any, + compName: string, + visited: WeakSet = new WeakSet(), + depth = 0 +): void { if (!instance || typeof instance !== 'object') return; - if (instance.__angularRenderScanPatchedSignals) return; - instance.__angularRenderScanPatchedSignals = true; - - let signalSymbol: symbol | undefined; - const getSignalSymbol = (obj: any) => { - if (!obj || (typeof obj !== 'object' && typeof obj !== 'function')) return undefined; - if (signalSymbol) return signalSymbol; - const sym = Reflect.ownKeys(obj).find(k => typeof k === 'symbol' && String(k).includes('SIGNAL')); - if (sym) signalSymbol = sym as symbol; - return signalSymbol; - }; + // Track visited objects in a side WeakSet instead of writing a marker onto the + // instance. Writing an enumerable property pollutes Object.keys() of any object + // we touch, which breaks Angular containers (form controls, URL segments). + if (visited.has(instance)) return; + visited.add(instance); const keys = Object.getOwnPropertyNames(instance); for (const key of keys) { @@ -68,7 +102,12 @@ function patchSignalsOnInstance(instance: any, compName: string): void { } } as any; - wrapped.__angularRenderScanPatched = true; + Object.defineProperty(wrapped, '__angularRenderScanPatched', { + value: true, + enumerable: false, + configurable: true, + writable: true + }); wrapped[sym] = internalNode; wrapped.toString = originalSignal.toString.bind(originalSignal); @@ -121,21 +160,23 @@ function patchSignalsOnInstance(instance: any, compName: string): void { instance[key] = wrapped; } - } else if (typeof prop === 'object' && prop !== null && !Array.isArray(prop)) { + } else if (depth < MAX_SIGNAL_PATCH_DEPTH && typeof prop === 'object' && prop !== null && !Array.isArray(prop)) { + // Only recurse into class instances that look like view models. Skip + // framework internals and plain-object containers (constructor `Object`), + // since those are the dictionaries Angular iterates with Object.keys(). const ctorName = prop.constructor?.name; if ( ctorName && + ctorName !== 'Object' && !ctorName.startsWith('ɵ') && - !ctorName.startsWith('ElementRef') && - !ctorName.startsWith('ViewContainerRef') && - !ctorName.startsWith('ChangeDetectorRef') && - !ctorName.startsWith('NgZone') && - !ctorName.startsWith('ApplicationRef') && - !ctorName.startsWith('Router') && - !ctorName.startsWith('ActivatedRoute') && - !(prop instanceof HTMLElement) + !FRAMEWORK_CTOR_EXCLUSIONS.some(name => ctorName.startsWith(name)) && + !(prop instanceof HTMLElement) && + !(prop instanceof Date) && + !(prop instanceof RegExp) && + !(prop instanceof Map) && + !(prop instanceof Set) ) { - patchSignalsOnInstance(prop, ctorName); + patchSignalsOnInstance(prop, ctorName, visited, depth + 1); } } }