fix: fix renderEcharts refresh issue #4741
Conversation
|
WalkthroughThe changes involve modifications to the Changes
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/effects/plugins/src/echarts/use-echarts.ts (2)
34-34: LGTM! Consider documenting the theme behavior.The change to return an empty object when not in dark mode helps prevent theme-specific options from persisting. This is a good fix for the refresh issue.
Consider adding a comment explaining the theme-specific behavior:
const getOptions = computed((): EChartsOption => { if (!isDark.value) { + // Return empty options for light theme to prevent theme-specific options from persisting return {}; }
Line range hint
58-75: Consider cleaning up pending timeouts on re-render.The Promise-based retry mechanism could lead to memory leaks if
renderEchartsis called multiple times in quick succession, as old timeout callbacks aren't cancelled.Consider this implementation:
+ let renderTimeout: ReturnType<typeof setTimeout> | null = null; const renderEcharts = (options: EChartsOption, clear = true) => { cacheOptions = options; const currentOptions = { ...options, ...getOptions.value, }; return new Promise((resolve) => { + // Clear any pending render + if (renderTimeout) { + clearTimeout(renderTimeout); + renderTimeout = null; + } if (chartRef.value?.offsetHeight === 0) { - useTimeoutFn(() => { + renderTimeout = setTimeout(() => { renderEcharts(currentOptions); resolve(null); }, 30); return; } nextTick(() => { - useTimeoutFn(() => { + renderTimeout = setTimeout(() => { if (!chartInstance) { const instance = initCharts(); if (!instance) return; } clear && chartInstance?.clear(); chartInstance?.setOption(currentOptions); resolve(null); + renderTimeout = null; }, 30); }); }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/effects/plugins/src/echarts/use-echarts.ts (3 hunks)
🔇 Additional comments (2)
packages/effects/plugins/src/echarts/use-echarts.ts (2)
54-57: LGTM! Good fix for the chart refresh issue.Creating a new
currentOptionsobject by merging the input options with computed options ensures that each render uses fresh data, fixing the chart refresh issue when rendering multiple times.Also applies to: 73-73
Line range hint
102-105: Verify chart cleanup and theme switching.The changes look good, but let's verify two critical paths:
- Chart cleanup on unmount
- Theme switching behavior
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to find any cleanup patterns in echarts-related files and theme-related code, along with examining the actual implementation of useEcharts to understand the context better.
Chart cleanup and theme switching are properly implemented
After examining the implementation, both concerns are well-addressed in the code:
Chart cleanup is properly handled:
chartInstance?.dispose()is called intryOnUnmountedhook- The instance is also properly disposed during theme changes before reinitializing
Theme switching is correctly implemented:
- Watches
isDarkfor theme changes- Properly reinitializes the chart with the new theme
- Preserves the chart options during theme switch using
cacheOptions- Includes resize handling after theme switch
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for proper cleanup in other components rg -l "useEcharts.*dispose" --type ts # Check for theme-related issues rg "isDark.*echarts" --type ts -A 3Length of output: 77
Script:
#!/bin/bash # Let's try a broader search for cleanup patterns in echarts-related files fd "echarts" --type f --extension ts --exec rg -l "dispose|cleanup|destroy" {} # Search for theme-related code in echarts files fd "echarts" --type f --extension ts --exec rg -l "theme|dark" {} # Get the actual implementation of useEcharts fd "use-echarts.ts" --type f --exec cat {}Length of output: 2820
anncwb
changed the title
Description
使用renderEcharts对同一图表在更新数据后重新渲染，实际图表并未发送变化。
原因: renderEcharts里getOptions.value值依旧是第一次渲染的值
Type of change
Please delete options that are not relevant.
Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
New Features
Bug Fixes
Documentation