Skip to content

fix: clone calcParams and figures on indicator override to stop reference aliasing - #850

Open
NemeZZiZZ wants to merge 1 commit into
klinecharts:mainfrom
NemeZZiZZ:fix/indicator-override-aliasing
Open

fix: clone calcParams and figures on indicator override to stop reference aliasing#850
NemeZZiZZ wants to merge 1 commit into
klinecharts:mainfrom
NemeZZiZZ:fix/indicator-override-aliasing

Conversation

@NemeZZiZZ

Copy link
Copy Markdown
Contributor

Problem

Indicator.override assigns calcParams and figures by reference, while every other field goes through merge/clone:

if (isValid(calcParams)) { this.calcParams = calcParams }   // caller's array
...
this.figures = figures ?? this.figures                      // caller's array

Consequences:

  1. Shared state across instances. IndicatorImp.extend(template) passes the template's arrays through override, so every instance of a registered indicator shares one calcParams array. Verified by execution: instance.calcParams === template.calcParams (strict reference equality).
  2. In-place mutation is invisible to shouldUpdate. _prevIndicator is snapshotted after the mutation target is already shared, so JSON.stringify(prev.calcParams) === JSON.stringify(current.calcParams) and no recalculation happens — the chart keeps showing stale values even though the params changed.

Fix

Assign clones instead of references:

this.calcParams = clone(calcParams)
...
if (isValid(figures)) { this.figures = clone(figures) }

clone returns non-objects (including the styles callback some figures carry) by reference, so function-valued figure properties are preserved.

Verification

Against the real typeChecks.ts on Node: cloned calcParams are value-equal but reference-distinct; mutating the caller's array after override no longer leaks into the instance; figure objects are copied while their styles callbacks are preserved by reference.

Note: a separate, related quirk — merge() copies explicitly-passed undefined (overrideIndicator({ id, visible: undefined }) clears visibility) — is in the shared merge utility and intentionally not bundled here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant