fix: useEcharts return invalid instance #5360
Conversation
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/effects/plugins/src/echarts/use-echarts.tsOops! Something went wrong! :( ESLint: 9.17.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
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: 1
🧹 Nitpick comments (1)
packages/effects/plugins/src/echarts/use-echarts.ts (1)
66-67: Extract timeout duration to a constant.The hardcoded timeout of 30ms is used in multiple places. Consider extracting it to a named constant for better maintainability.
+const RENDER_TIMEOUT_MS = 30; + const renderEcharts = (/*...*/) => { // ... useTimeoutFn(async () => { resolve(await renderEcharts(currentOptions)); - }, 30); + }, RENDER_TIMEOUT_MS); // ... useTimeoutFn(() => { // ... resolve(chartInstance); - }, 30); + }, RENDER_TIMEOUT_MS);Also applies to: 79-79
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/effects/plugins/src/echarts/use-echarts.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: CodeQL
- GitHub Check: Lint (windows-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/effects/plugins/src/echarts/use-echarts.ts (3)
5-5: LGTM! Type import enhances type safety.The addition of the
Nullabletype import improves type safety for the chart instance handling.
116-116: LGTM! Good encapsulation practice.The addition of
getChartInstancegetter provides better encapsulation of the chart instance and follows Vue.js best practices.
Line range hint
55-82: Verify usage of useEcharts in the codebase.Let's ensure that all consumers of
useEchartsare prepared for the new return type and getter method.Also applies to: 116-116
✅ Verification successful
No breaking changes detected in useEcharts consumers
All current usages only utilize the
renderEchartsmethod in a fire-and-forget style withinonMounted, with no dependency on the return value or direct chart instance access. The changes are safe to proceed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for useEcharts usage patterns echo "Checking direct chartInstance access (should be updated to use getter):" rg "const.*=.*useEcharts\(" -A 5 echo "Checking renderEcharts promise handling (should handle nullable return):" rg "renderEcharts\(.*\).*then\(" -A 2Length of output: 10861
Description
修复useEcharts返回的instance值可能不正确的问题
fix #5354
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
New Features
Refactor