diff --git a/packages/chart/src/core/chart.ts b/packages/chart/src/core/chart.ts index 67bf7ec..24a0e25 100644 --- a/packages/chart/src/core/chart.ts +++ b/packages/chart/src/core/chart.ts @@ -284,6 +284,19 @@ export interface ScaleApi { /** * A chart instance. Create via {@link createChart}. */ +function assertFinitePoint(point: { index?: number; price?: number }): void { + if (!(Number.isFinite(point.index) && Number.isFinite(point.price))) { + throw new Error('chart-ts: drawing point index and price must be finite'); + } +} + +function assertDrawingPoints(name: string, minPoints: number, points: readonly { index?: number; price?: number }[]): void { + if (points.length < minPoints) { + throw new Error(`chart-ts: drawing "${name}" needs at least ${minPoints} point${minPoints === 1 ? '' : 's'}`); + } + for (const point of points) assertFinitePoint(point); +} + export class Chart { /** Indicator registry; register custom indicators here. */ readonly indicators: IndicatorRegistry; @@ -407,6 +420,7 @@ export class Chart { } else { this.ready = Promise.resolve(this); } + this.assertConfigIds(); this.render(); if (options.autoResize === true) this.startAutoResize(); } @@ -563,6 +577,8 @@ export class Chart { /** Deep-merges a partial over the current config and re-renders. */ updateConfig(partial: DeepPartial): void { + if (partial.indicators !== undefined) this.assertIndicatorList(partial.indicators); + if (partial.drawings !== undefined) this.assertDrawingList(partial.drawings); if (partial.width !== undefined) assertLayoutLength(partial.width, 'width'); if (partial.height !== undefined) assertLayoutLength(partial.height, 'height'); if (partial.priceAxis?.width !== undefined) assertLayoutLength(partial.priceAxis.width, 'priceAxis.width'); @@ -672,6 +688,40 @@ export class Chart { if (this.batchDepth === 0) this.events.flush(); } + /** Rejects duplicate ids and indicator ids that share the pane namespace. */ + private assertConfigIds(): void { + this.assertIndicatorList(this.config.indicators); + this.assertDrawingList(this.config.drawings); + } + + private assertNewIndicatorId(id: string): void { + if (id === 'main' || this.customPanes.has(id)) throw new Error(`chart-ts: indicator id "${id}" collides with a pane`); + if (this.config.indicators.some((ind) => ind.id === id)) throw new Error(`chart-ts: duplicate indicator "${id}"`); + } + + private assertIndicatorList(indicators: readonly { id?: string }[]): void { + const seen = new Set(); + for (const ind of indicators) { + const id = ind.id ?? ''; + if (id === 'main' || this.customPanes.has(id)) throw new Error(`chart-ts: indicator id "${id}" collides with a pane`); + if (seen.has(id)) throw new Error(`chart-ts: duplicate indicator "${id}"`); + seen.add(id); + } + } + + private assertDrawingList(drawings: readonly { id?: string; name?: string; points?: readonly { index?: number; price?: number }[] }[]): void { + const seen = new Set(); + for (const drawing of drawings) { + const id = drawing.id ?? ''; + if (seen.has(id)) throw new Error(`chart-ts: duplicate drawing "${id}"`); + seen.add(id); + const points = drawing.points ?? []; + const def = drawing.name === undefined ? undefined : this.drawings.get(drawing.name); + if (def !== undefined) assertDrawingPoints(def.name, def.minPoints, points); + else for (const point of points) assertFinitePoint(point); + } + } + /** * Adds an indicator instance. Returns its id. * @@ -680,8 +730,10 @@ export class Chart { addIndicator(input: AddIndicatorInput): string { const def = this.indicators.get(input.name); if (def === undefined) throw new Error(`chart-ts: unknown indicator "${input.name}"`); + const id = input.id ?? `ind-${++this.indicatorSeq}`; + this.assertNewIndicatorId(id); const cfg: IndicatorConfig = { - id: input.id ?? `ind-${++this.indicatorSeq}`, + id, name: def.name, params: { ...def.defaultParams, ...input.params }, pane: input.pane ?? def.defaultPane, @@ -754,6 +806,9 @@ export class Chart { autoscale: options.autoscale, primitives: [], }; + if (state.id === 'main' || this.config.indicators.some((ind) => ind.id === state.id)) { + throw new Error(`chart-ts: pane id "${state.id}" collides with a pane`); + } if (this.customPanes.has(state.id)) throw new Error(`chart-ts: duplicate pane "${state.id}"`); this.customPanes.set(state.id, state); this.render(); @@ -838,8 +893,11 @@ export class Chart { */ addDrawing(input: AddDrawingInput): string { const def = this.requireDrawing(input.name); + assertDrawingPoints(def.name, def.minPoints, input.points); + const id = input.id ?? `drw-${++this.drawingSeq}`; + if (this.config.drawings.some((drawing) => drawing.id === id)) throw new Error(`chart-ts: duplicate drawing "${id}"`); const cfg: DrawingConfig = { - id: input.id ?? `drw-${++this.drawingSeq}`, + id, name: def.name, points: def.normalize !== undefined ? def.normalize(input.points) : input.points, color: input.color ?? DEFAULT_DRAWING_COLOR, @@ -865,6 +923,11 @@ export class Chart { updateDrawing(id: string, patch: DrawingPatch): boolean { const cfg = this.getDrawing(id); if (cfg === undefined) return false; + if (patch.points !== undefined) { + const def = this.drawings.get(cfg.name); + if (def !== undefined) assertDrawingPoints(def.name, def.minPoints, patch.points); + else for (const point of patch.points) assertFinitePoint(point); + } Object.assign(cfg, patch); this.events.configChanged('drawings'); this.render(); @@ -1019,6 +1082,7 @@ export class Chart { moveDrawingPoint(id: string, pointIndex: number, point: DrawingPoint): boolean { const cfg = this.getDrawing(id); if (cfg === undefined || pointIndex < 0 || pointIndex >= cfg.points.length) return false; + assertFinitePoint(point); cfg.points = cfg.points.map((p, i) => (i === pointIndex ? point : p)); this.events.configChanged('drawings'); this.render(); diff --git a/packages/chart/test/chart.test.ts b/packages/chart/test/chart.test.ts index 87a559d..009dfea 100644 --- a/packages/chart/test/chart.test.ts +++ b/packages/chart/test/chart.test.ts @@ -135,9 +135,9 @@ describe('Chart API', () => { assert.equal(chart.removeDrawing('nope'), false); chart.destroy(); }); - it('skips drawings with too few points or unregistered leftovers', () => { + it('rejects drawings with too few points and skips unregistered leftovers', () => { const chart = createChart({ document: new MockDocument(), config: { wasm: false, data: candles(10) } }); - chart.addDrawing({ name: 'trendline', points: [{ index: 1, price: 100 }] }); + assert.throws(() => chart.addDrawing({ name: 'trendline', points: [{ index: 1, price: 100 }] }), /at least 2 points/); chart.getConfig().drawings.push({ id: 'x', name: 'ghost', points: [], color: '#000', lineWidth: 1, lineStyle: 'solid', text: '', image: null, locked: false, visible: true }); chart.render(); chart.destroy(); diff --git a/packages/chart/test/drawing-id-invariants.test.ts b/packages/chart/test/drawing-id-invariants.test.ts new file mode 100644 index 0000000..7f73e33 --- /dev/null +++ b/packages/chart/test/drawing-id-invariants.test.ts @@ -0,0 +1,92 @@ +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; +import { createChart, type Chart } from '../dist/index.js'; +import { MockDocument } from '../dist/dom.js'; + +function chart(): Chart { + return createChart({ + document: new MockDocument(), + config: { + wasm: false, + data: Array.from({ length: 30 }, (_, i) => ({ time: 1_700_000_000 + i * 60, open: 10, high: 12, low: 9, close: 11 })), + }, + }); +} + +describe('drawing points', () => { + it('rejects non-finite points and too few points before changing the chart (#95)', () => { + const c = chart(); + assert.throws(() => c.addDrawing({ id: 'sp', name: 'signpost', points: [{ index: Number.NaN, price: 100 }] }), /index and price must be finite/); + assert.throws(() => c.addDrawing({ name: 'hline', points: [{ index: 1, price: Number.POSITIVE_INFINITY }] }), /index and price must be finite/); + assert.throws(() => c.addDrawing({ name: 'long-position', points: [] }), /long-position" needs at least 1 point$/); + assert.throws(() => c.addDrawing({ name: 'trendline', points: [{ index: 1, price: 1 }] }), /trendline" needs at least 2 points$/); + assert.deepEqual(c.getConfig().drawings, []); + c.setData(c.getData().slice()); + c.render(); + + const id = c.addDrawing({ name: 'hline', points: [{ index: 2, price: 11 }] }); + assert.throws(() => c.updateDrawing(id, { points: [{ index: Number.NaN, price: 11 }] }), /must be finite/); + assert.throws(() => c.moveDrawingPoint(id, 0, { index: 2, price: Number.NaN }), /must be finite/); + assert.deepEqual(c.getDrawing(id)?.points, [{ index: 2, price: 11 }]); + assert.equal(c.updateDrawing('missing', { color: '#fff' }), false); + c.destroy(); + + assert.throws(() => createChart({ + document: new MockDocument(), + config: { wasm: false, drawings: [{ id: 'sp', name: 'signpost', points: [{ index: Number.NaN, price: 1 }], color: '#fff', lineWidth: 1, lineStyle: 'solid', text: '', image: null, locked: false, visible: true }] }, + }), /must be finite/); + const ghost = createChart({ + document: new MockDocument(), + config: { wasm: false, drawings: [{ id: 'g', name: 'custom-model', points: [{ index: 1, price: 2 }], color: '#fff', lineWidth: 1, lineStyle: 'solid', text: '', image: null, locked: false, visible: true }] }, + }); + assert.equal(ghost.getConfig().drawings.length, 1); + ghost.destroy(); + }); +}); + +describe('indicator and drawing ids', () => { + it('rejects duplicate ids and indicator ids that collide with panes (#96)', () => { + const c = chart(); + c.addIndicator({ id: 'x', name: 'sma', params: { period: 1 } }); + assert.throws(() => c.addIndicator({ id: 'x', name: 'sma', params: { period: 2 } }), /duplicate indicator "x"/); + assert.equal(c.getConfig().indicators.length, 1); + assert.throws(() => c.addIndicator({ id: 'main', name: 'rsi' }), /indicator id "main" collides with a pane/); + c.addPane({ id: 'book' }); + assert.throws(() => c.addIndicator({ id: 'book', name: 'rsi' }), /indicator id "book" collides with a pane/); + c.addIndicator({ id: 'study', name: 'ema' }); + assert.throws(() => c.addPane({ id: 'study' }), /pane id "study" collides with a pane/); + assert.throws(() => c.addPane({ id: 'main' }), /pane id "main" collides with a pane/); + + c.addDrawing({ id: 'd', name: 'hline', points: [{ index: 1, price: 11 }] }); + assert.throws(() => c.addDrawing({ id: 'd', name: 'hline', points: [{ index: 2, price: 12 }] }), /duplicate drawing "d"/); + assert.equal(c.getConfig().drawings.length, 1); + c.updateDrawing('d', { color: '#f00' }); + assert.equal(c.getDrawing('d')?.color, '#f00'); + + assert.throws(() => c.updateConfig({ + indicators: [ + { id: 'a', name: 'sma', params: {}, pane: 'main', colors: [], visible: true }, + { id: 'a', name: 'sma', params: {}, pane: 'main', colors: [], visible: true }, + ], + }), /duplicate indicator "a"/); + assert.equal(c.getConfig().indicators.length, 2); + assert.throws(() => c.updateConfig({ + drawings: [ + { id: 'd', name: 'hline', points: [{ index: 1, price: 1 }], color: '#fff', lineWidth: 1, lineStyle: 'solid', text: '', image: null, locked: false, visible: true }, + { id: 'd', name: 'hline', points: [{ index: 2, price: 2 }], color: '#fff', lineWidth: 1, lineStyle: 'solid', text: '', image: null, locked: false, visible: true }, + ], + }), /duplicate drawing "d"/); + assert.equal(c.getConfig().drawings.length, 1); + c.destroy(); + + assert.throws(() => createChart({ + document: new MockDocument(), + config: { + wasm: false, + indicators: [ + { id: 'main', name: 'sma', params: {}, pane: 'sub', colors: [], visible: true }, + ], + }, + }), /indicator id "main" collides with a pane/); + }); +});