Skip to content

Commit 15e7fe9

Browse files
committed
fix(milkdown): consolidate chart mounting and cleanup
1 parent 1c02c5f commit 15e7fe9

8 files changed

Lines changed: 562 additions & 217 deletions

File tree

‎packages/cherry-markdown/src/addons/advance/cherry-table-echarts-plugin.js‎

Lines changed: 12 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import mergeWith from '@/utils/toolkit/mergeWith';
1717
import Logger from '@/Logger';
1818
import { getExternal } from '@/utils/external';
1919
import { isBrowser } from '@/utils/env';
20-
import defaultLocale from '@/locales/zh_CN';
2120

2221
// 主题与常量集中管理
2322
const THEME = {
@@ -88,7 +87,7 @@ export default class EChartsTableEngine {
8887
// 保存Cherry配置,用于获取地图数据源URL
8988
this.cherryOptions = cherryOptions;
9089
// 保存Cherry实例,用于事件监听及i18n
91-
this.cherry = cherry || { locale: defaultLocale };
90+
this.cherry = cherry;
9291
// 统一管理实例
9392
this.instances = new Set();
9493
// 主题监听器
@@ -806,50 +805,6 @@ export default class EChartsTableEngine {
806805
return htmlContent;
807806
}
808807

809-
/**
810-
* Mount a table chart into a caller-owned container.
811-
*
812-
* Unlike render(), this entry does not depend on Cherry Previewer's DOM or
813-
* delayed global lookup. It lets integrations reuse Cherry's complete chart
814-
* semantics while retaining ownership of their own node lifecycle.
815-
*/
816-
renderInto(container, type, options, tableObject, signal) {
817-
if (!container || !this.echartsRef) return () => {};
818-
const lifecycleController = new AbortController();
819-
const abortLifecycle = () => lifecycleController.abort();
820-
signal?.addEventListener('abort', abortLifecycle, { once: true });
821-
const chartId = container.id || `chart-${Date.now()}-${Math.random().toString(36).slice(2, 11)}`;
822-
const chartOptions = { ...(options || {}), chartId, signal: lifecycleController.signal };
823-
const serializableOptions = { ...(options || {}), chartId };
824-
container.id = chartId;
825-
container.classList.add('cherry-echarts-wrapper');
826-
container.dataset.chartType = type;
827-
container.dataset.tableData = JSON.stringify(tableObject);
828-
container.dataset.chartOptions = JSON.stringify(serializableOptions);
829-
if (!container.style.width) container.style.width = '100%';
830-
if (!container.style.height) container.style.height = `${this.options.height}px`;
831-
this.$buildEchartsThemeFromCss(container);
832-
const chart = this.createChart(container, this.$generateChartOptions(type, tableObject, chartOptions), type);
833-
const observer = typeof ResizeObserver === 'undefined' ? null : new ResizeObserver(() => chart?.resize?.());
834-
observer?.observe(container);
835-
let disposed = false;
836-
const cleanup = () => {
837-
if (disposed) return;
838-
disposed = true;
839-
signal?.removeEventListener('abort', abortLifecycle);
840-
lifecycleController.abort();
841-
observer?.disconnect();
842-
this.destroyChart(chart);
843-
if (this.instances.size === 0) {
844-
this.themeObservers.forEach((themeObserver) => themeObserver.disconnect());
845-
this.themeObservers.clear();
846-
this.exportObservers.forEach((handler) => window.removeEventListener('cherry:export:done', handler));
847-
this.exportObservers.clear();
848-
}
849-
};
850-
return cleanup;
851-
}
852-
853808
// 添加点击高亮效果
854809
addClickHighlightEffect(chartInstance, chartType) {
855810
let selectedDataIndex = null;
@@ -1663,6 +1618,9 @@ const MapChartFailureOptionsHandler = {
16631618
},
16641619
};
16651620

1621+
// Preserve optional cancellation when a map is rebuilt from its container data.
1622+
const mapRequestSignals = new WeakMap();
1623+
16661624
const MapChartOptionsHandler = {
16671625
options(tableObject, options) {
16681626
// 获取当前用户指定的地图数据源
@@ -1745,18 +1703,22 @@ const MapChartOptionsHandler = {
17451703
}
17461704

17471705
const url = paths[index];
1706+
const container = document.querySelector(`[id="${options.chartId}"][data-chart-type="map"]`);
1707+
const signal = options.signal ?? (container ? mapRequestSignals.get(container) : undefined);
1708+
if (container && signal) mapRequestSignals.set(container, signal);
1709+
if (signal?.aborted) return;
17481710
// console.log(`尝试加载地图数据: ${url}`);
17491711

1750-
this.$fetchMapData(url, options.signal)
1712+
this.$fetchMapData(url, signal)
17511713
.then((geoJson) => {
1752-
if (options.signal?.aborted) return geoJson;
1714+
if (signal?.aborted) return geoJson;
17531715
options.engine.echartsRef?.registerMap?.(url, geoJson);
17541716
// console.log(`地图数据加载成功!来源: ${url}`);
17551717
this.$refreshMapChart(options.chartId, url, options.engine);
17561718
return geoJson;
17571719
})
17581720
.catch((error) => {
1759-
if (error?.name === 'AbortError' || options.signal?.aborted) return;
1721+
if (error?.name === 'AbortError' || signal?.aborted) return;
17601722
Logger.warn(`Map data loading failed (${url}):`, error.message);
17611723
this.$handleMapLoadFailure(options);
17621724
});
@@ -1790,7 +1752,7 @@ const MapChartOptionsHandler = {
17901752
}
17911753
},
17921754
async $fetchMapData(url, signal) {
1793-
const response = await fetch(url, { referrerPolicy: 'no-referrer', signal });
1755+
const response = await fetch(url, { referrerPolicy: 'no-referrer', ...(signal ? { signal } : {}) });
17941756
if (!response.ok) {
17951757
throw new Error(`HTTP error! status: ${response.status} for ${url}`);
17961758
}

‎packages/cherry-markdown/test/addons/table-echarts.spec.ts‎

Lines changed: 128 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -95,31 +95,6 @@ describe('addons/EChartsTableEngine', () => {
9595
expect(renderer.instances.size).toBe(0);
9696
});
9797

98-
it('mounts into a caller-owned container and releases all retained resources', () => {
99-
const environment = createEnvironment();
100-
const renderer = new EChartsTableEngine({ echarts: environment.echarts, cherry: environment.cherry });
101-
const container = document.createElement('div');
102-
environment.root.appendChild(container);
103-
const controller = new AbortController();
104-
105-
const cleanup = renderer.renderInto(
106-
container,
107-
'line',
108-
{ title: 'Mounted' },
109-
{ header: ['', 'Q1'], rows: [['Sales', '1']] },
110-
controller.signal,
111-
);
112-
113-
expect(environment.echarts.init).toHaveBeenCalledWith(container, null, renderer.options);
114-
expect(container.dataset.chartType).toBe('line');
115-
expect(container.dataset.tableData).toContain('Sales');
116-
expect(renderer.instances.size).toBe(1);
117-
cleanup();
118-
cleanup();
119-
expect(environment.chart.dispose).toHaveBeenCalledOnce();
120-
expect(renderer.instances.size).toBe(0);
121-
});
122-
12398
it('provides chart palettes, axes, zoom controls, and numeric normalization', () => {
12499
const environment = createEnvironment();
125100
const renderer = new EChartsTableEngine({ echarts: environment.echarts, cherry: environment.cherry });
@@ -578,6 +553,134 @@ describe('addons/EChartsTableEngine', () => {
578553
expect(inferredGroup.series[0].name).toBe('系列1');
579554
});
580555

556+
it.each(['global', 'injected', 'both'])('uses the selected ECharts instance for maps (%s)', (mode) => {
557+
const environment = createEnvironment();
558+
const globalEcharts = { getMap: vi.fn(), registerMap: vi.fn() };
559+
environment.echarts.getMap.mockReturnValue({});
560+
if (mode === 'global') Object.assign(window, { echarts: environment.echarts });
561+
else if (mode === 'both') window.echarts = globalEcharts as unknown as typeof window.echarts;
562+
const fetchMock = vi.fn();
563+
vi.stubGlobal('fetch', fetchMock);
564+
const renderer = new EChartsTableEngine({
565+
...(mode === 'global' ? {} : { echarts: environment.echarts }),
566+
cherry: environment.cherry,
567+
});
568+
renderer.$buildEchartsThemeFromCss(environment.root);
569+
570+
const option = renderer.$generateChartOptions(
571+
'map',
572+
{ header: ['Province', 'Value'], rows: [['北京', '10']] },
573+
{ mapDataSource: 'china' },
574+
);
575+
576+
expect(option.series[0]).toMatchObject({ type: 'map', map: 'china' });
577+
expect(environment.echarts.getMap).toHaveBeenCalledWith('china');
578+
expect(globalEcharts.getMap).not.toHaveBeenCalled();
579+
expect(fetchMock).not.toHaveBeenCalled();
580+
});
581+
582+
it('registers fetched maps on the injected instance, not a different global instance', async () => {
583+
const environment = createEnvironment();
584+
const globalEcharts = { getMap: vi.fn(), registerMap: vi.fn() };
585+
window.echarts = globalEcharts as unknown as typeof window.echarts;
586+
const geoJson = { type: 'FeatureCollection', features: [] };
587+
vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: true, json: async () => geoJson }));
588+
const renderer = new EChartsTableEngine({ echarts: environment.echarts, cherry: environment.cherry });
589+
renderer.$buildEchartsThemeFromCss(environment.root);
590+
const tableData = { header: ['Province', 'Value'], rows: [['北京', '10']] };
591+
const container = environment.previewerDom.appendChild(document.createElement('div'));
592+
container.id = 'map-chart';
593+
container.dataset.chartType = 'map';
594+
container.dataset.tableData = JSON.stringify(tableData);
595+
container.dataset.chartOptions = JSON.stringify({ mapDataSource: 'china' });
596+
597+
renderer.$generateChartOptions('map', tableData, { mapDataSource: 'china', chartId: container.id });
598+
599+
await vi.waitFor(() => expect(container.dataset.mapStatus).toBe('success'));
600+
expect(environment.echarts.registerMap).toHaveBeenCalledWith('china', geoJson);
601+
expect(globalEcharts.getMap).not.toHaveBeenCalled();
602+
expect(globalEcharts.registerMap).not.toHaveBeenCalled();
603+
renderer.onDestroy();
604+
});
605+
606+
it.each(['request', 'rebuild'])('ignores late map responses after optional %s cancellation', async (scope) => {
607+
const environment = createEnvironment();
608+
const controller = new AbortController();
609+
const pending: Array<(value: Response) => void> = [];
610+
const fetchMock = vi.fn(() => new Promise<Response>((resolve) => pending.push(resolve)));
611+
vi.stubGlobal('fetch', fetchMock);
612+
const renderer = new EChartsTableEngine({
613+
echarts: environment.echarts,
614+
cherry: environment.cherry,
615+
});
616+
renderer.$buildEchartsThemeFromCss(environment.root);
617+
const container = environment.previewerDom.appendChild(document.createElement('div'));
618+
container.id = 'cancelled-map';
619+
container.dataset.chartType = 'map';
620+
const tableData = { header: ['Province', 'Value'], rows: [['北京', '10']] };
621+
container.dataset.tableData = JSON.stringify(tableData);
622+
container.dataset.chartOptions = JSON.stringify({ mapDataSource: 'china' });
623+
renderer.$generateChartOptions('map', tableData, {
624+
mapDataSource: 'china',
625+
chartId: container.id,
626+
signal: controller.signal,
627+
});
628+
if (scope === 'rebuild') renderer.$chartOptionsFromDataset(container);
629+
expect(fetchMock).toHaveBeenCalledTimes(scope === 'rebuild' ? 2 : 1);
630+
expect(fetchMock).toHaveBeenCalledWith('china', {
631+
referrerPolicy: 'no-referrer',
632+
signal: controller.signal,
633+
});
634+
635+
controller.abort();
636+
pending.forEach((finish) => finish(new Response(JSON.stringify({ type: 'FeatureCollection', features: [] }))));
637+
await new Promise((resolve) => setTimeout(resolve, 0));
638+
639+
expect(environment.echarts.registerMap).not.toHaveBeenCalled();
640+
expect(environment.echarts.init).not.toHaveBeenCalled();
641+
expect(container.dataset.mapStatus).toBe('loading');
642+
});
643+
644+
it('does not inherit an engine-level cancellation signal for an ordinary map request', () => {
645+
const environment = createEnvironment();
646+
const controller = new AbortController();
647+
controller.abort();
648+
const fetchMock = vi.fn(() => new Promise(() => {}));
649+
vi.stubGlobal('fetch', fetchMock);
650+
const renderer = new EChartsTableEngine({
651+
echarts: environment.echarts,
652+
cherry: environment.cherry,
653+
signal: controller.signal,
654+
});
655+
renderer.$buildEchartsThemeFromCss(environment.root);
656+
657+
renderer.$generateChartOptions(
658+
'map',
659+
{ header: ['Province', 'Value'], rows: [['北京', '10']] },
660+
{ mapDataSource: 'china' },
661+
);
662+
663+
expect(fetchMock).toHaveBeenCalledWith('china', { referrerPolicy: 'no-referrer' });
664+
});
665+
666+
it('does not start a map request for an already cancelled signal', () => {
667+
const environment = createEnvironment();
668+
const controller = new AbortController();
669+
controller.abort();
670+
const fetchMock = vi.fn();
671+
vi.stubGlobal('fetch', fetchMock);
672+
const renderer = new EChartsTableEngine({ echarts: environment.echarts, cherry: environment.cherry });
673+
renderer.$buildEchartsThemeFromCss(environment.root);
674+
675+
renderer.$generateChartOptions(
676+
'map',
677+
{ header: ['Province', 'Value'], rows: [['北京', '10']] },
678+
{ mapDataSource: 'china', signal: controller.signal },
679+
);
680+
681+
expect(fetchMock).not.toHaveBeenCalled();
682+
});
683+
581684
it('generates a registered map with normalized province names', () => {
582685
const environment = createEnvironment();
583686
environment.echarts.getMap.mockReturnValue({});

‎packages/milkdown/ARCHITECTURE.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
- `editor.ts` owns the Milkdown lifecycle and public instance API.
66
- `CherryEngine.makeHtml(markdown)` renders Cherry-owned native syntax; registered visual renderers handle Mermaid, ECharts, and custom fenced blocks.
77
- Cherry dist imports are isolated behind `cherry-engine.ts`, `cherry-echarts.ts`, and `cherry-codeblock-echarts.ts`; these are the package-upgrade checkpoints. The published Cherry stylesheet remains an explicit CSS dependency.
8-
- The table-chart adapter prefers Cherry's `renderInto()` when present. The declared minimum (0.11.10) instead supplies the existing chart option generator and lifecycle methods; the adapter owns only its mount/cleanup, not chart conversion or Previewer DOM. Consumer checks exercise both the published minimum and the workspace tarball.
8+
- Table and fenced charts share one Milkdown-owned mount/cleanup path. It uses Cherry's existing option generator and chart lifecycle, including the declared minimum (0.11.10), without adding a Cherry mounting API or depending on Previewer DOM. Charts in one editor share a reference-counted engine, matching Cherry's root-scoped export/theme observers; each mount owns its request-cancellation scope. Cleanup and resize resolve the current chart after export rebuilds, and the final mount releases the engine. Consumer checks exercise both the published minimum and the workspace tarball.
99
- Its legacy locale context is generated from Cherry's chart messages during build (`sync-chart-locale.mjs`), not an independently maintained translation table. It does not import Cherry's editor/toolbar runtime to obtain translations.
1010
- Markdown source reconciliation accepts a formatting-preserving edit only after parser/serializer validation. Ambiguous matches fall back to canonical serialization rather than changing the wrong occurrence. Nested renderer results share an abort-owned cleanup scope, including late results after sibling failures.
1111
- `table-focus.ts` isolates Table Block's pointer/word selection integration. Table editing and controls remain Milkdown-owned; selection updates use ProseMirror transactions, never replacement of component DOM.

‎packages/milkdown/e2e/lifecycle-boundaries.spec.ts‎

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { test, expect, markdown, setMarkdown } from './fixtures';
1+
import { test, expect, markdown, replaceTableCell, setMarkdown } from './fixtures';
22
import type { Page } from '@playwright/test';
33

44
async function placeCaretAtTextOffset(page: Page, text: string, offset: number) {
@@ -87,6 +87,51 @@ test('theme changes refresh charts without replacing table/code chart DOM or the
8787
expect(await markdown(page)).toBe(value);
8888
});
8989

90+
test('export rebuilds preserve chart content, editing and deletion for both chart kinds', async ({ page }) => {
91+
const codeSource =
92+
'{animation:false,title:{text:"Code chart"},xAxis:{data:["Jan","Feb"]},yAxis:{},series:[{type:"line",data:[1,2]}]}';
93+
await setMarkdown(
94+
page,
95+
'| :line:{"title":"Table chart"} | Jan | Feb |\n| --- | --- | --- |\n| Sales | 1 | 2 |\n\n' +
96+
`\`\`\`echarts\n${codeSource}\n\`\`\``,
97+
);
98+
const table = page.locator('.cherry-table-chart');
99+
const code = page.locator('.cherry-embed--cherry_diagram:not([data-type="mermaid"])');
100+
await expect(table.locator('svg')).toContainText('Table chart');
101+
await expect(code.locator('svg')).toContainText('Code chart');
102+
for (let revision = 0; revision < 3; revision += 1) {
103+
await page.evaluate(() => window.dispatchEvent(new Event('cherry:export:done')));
104+
await expect(table.locator('svg')).toContainText('Table chart');
105+
await expect(code.locator('svg')).toContainText('Code chart');
106+
}
107+
108+
await replaceTableCell(page, page.locator('.milkdown-table-block td p').nth(1), '7');
109+
await expect(table.locator('.cherry-echarts-wrapper')).toHaveAttribute('data-table-data', /"Sales","7","2"/);
110+
await code.getByRole('button', { name: '在节点内编辑源码' }).click();
111+
const source = code.locator('.cherry-embed__source code');
112+
await source.click();
113+
await page.keyboard.press('ControlOrMeta+a');
114+
await page.keyboard.type(codeSource.replace('Code chart', 'Code updated'));
115+
await expect(code.locator('svg')).toContainText('Code updated');
116+
await page.evaluate(() => window.dispatchEvent(new Event('cherry:export:done')));
117+
await expect(code.locator('svg')).toContainText('Code updated');
118+
await expect(table.locator('.cherry-echarts-wrapper')).toHaveAttribute('data-table-data', /"Sales","7","2"/);
119+
120+
await setMarkdown(page, '| :line:{"title":"Table chart"} | Jan | Feb |\n| --- | --- | --- |\n| Sales | 7 | 2 |');
121+
await expect(code).toHaveCount(0);
122+
await expect(table.locator('svg')).toContainText('Table chart');
123+
await replaceTableCell(page, page.locator('.milkdown-table-block td p').nth(1), '9');
124+
await expect(table.locator('.cherry-echarts-wrapper')).toHaveAttribute('data-table-data', /"Sales","9","2"/);
125+
await page.evaluate(() => window.dispatchEvent(new Event('cherry:export:done')));
126+
await expect(table.locator('svg')).toContainText('Table chart');
127+
128+
await setMarkdown(page, 'Charts removed');
129+
await expect(page.locator('.cherry-echarts-wrapper')).toHaveCount(0);
130+
await page.evaluate(() => window.dispatchEvent(new Event('cherry:export:done')));
131+
await expect(page.locator('.cherry-echarts-wrapper')).toHaveCount(0);
132+
expect(await markdown(page)).toBe('Charts removed');
133+
});
134+
90135
test('native Chromium composition commits Chinese without duplicate text and supports undo', async ({
91136
page,
92137
browserName,

‎packages/milkdown/src/cherry-dist.d.ts‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,20 +16,14 @@ declare module 'cherry-markdown/dist/addons/advance/cherry-table-echarts-plugin.
1616
}
1717

1818
const EChartsTableEngine: new (options: Record<string, unknown>) => {
19-
renderInto?(
20-
container: HTMLElement,
21-
type: string,
22-
options: Record<string, unknown>,
23-
table: TableChartData,
24-
signal?: AbortSignal,
25-
): () => void;
2619
$buildEchartsThemeFromCss(container: HTMLElement): void;
2720
$generateChartOptions(
2821
type: string,
2922
table: TableChartData,
3023
options: Record<string, unknown>,
3124
): Record<string, unknown>;
3225
createChart(container: HTMLElement, options: Record<string, unknown>, type: string): { resize(): void } | null;
26+
destroyChart(container: HTMLElement): void;
3327
$applyThemeOnly(instance: { resize(): void } | null): void;
3428
onDestroy(): void;
3529
};

0 commit comments

Comments
 (0)