Skip to content

chore(charts): make echarts optional #1095

Open
abulte wants to merge 4 commits into
mainfrom
chore/chart-entrypoint
Open

chore(charts): make echarts optional #1095
abulte wants to merge 4 commits into
mainfrom
chore/chart-entrypoint

Conversation

@abulte
Copy link
Copy Markdown
Contributor

@abulte abulte commented May 25, 2026

Make echarts optional by declaring it as a peer dependency and using a dedicated entry point for its usage. Two benefits:

  1. Downstream that doesn't need the echarts components does not install the huge library
  2. Avoid downstream issues for local install of components. The chart components imports in main.ts will trigger vue deps analysis until echarts is reached, and it might not be installed (it's not in front-kit since we don't need it yet).
[plugin:vite:import-analysis] Failed to resolve import "echarts/core" from "node_modules/@datagouv/components-next/src/components/Chart/ChartViewer.vue". Does the file exist?
/Users/alexandre/Developer/Ecolab/Ecopshères/ecospheres-front/node_modules/@datagouv/components-next/src/components/Chart/ChartViewer.vue:9:48
12 |  /* Injection by vite-plugin-vue-inspector End */
13 |  import { defineComponent as _defineComponent } from "vue";
14 |  import { format, use } from "echarts/core";
   |                               ^
15 |  import { CanvasRenderer } from "echarts/renderers";
16 |  import { LineChart, BarChart } from "echarts/charts";

I went with a non-optional peer dep so that downstream gets a non-blocking warning inviting to install echarts if needed.

@abulte abulte marked this pull request as ready for review May 25, 2026 14:05
@ThibaudDauce
Copy link
Copy Markdown
Contributor

Why echarts is not installed? If it's used inside @datagouv/components-next it should be installed as a dependency for @datagouv/components-next, no?

@abulte
Copy link
Copy Markdown
Contributor Author

abulte commented May 26, 2026

Why echarts is not installed? If it's used inside @datagouv/components-next it should be installed as a dependency for @datagouv/components-next, no?

Good question! I'm not sure I get it completely, but looks like a combination of source install here + pnpm install downstream. I'm actually happy if it's not installed downstream for now.

BTW, it could be cleaner/more deterministic to declare echarts as a peerDependencies here. The charts endpoint would still make sense.

@ThibaudDauce
Copy link
Copy Markdown
Contributor

Why echarts is not installed? If it's used inside @datagouv/components-next it should be installed as a dependency for @datagouv/components-next, no?

Good question! I'm not sure I get it completely, but looks like a combination of source install here + pnpm install downstream. I'm actually happy if it's not installed downstream for now.

BTW, it could be cleaner/more deterministic to declare echarts as a peerDependencies here. The charts endpoint would still make sense.

I let @nicolaskempf57 decide, but I'm not a big fan of adding an entrypoint… It adds some asymmetries to the package for a random feature (charts).

@abulte
Copy link
Copy Markdown
Contributor Author

abulte commented May 26, 2026

Why echarts is not installed? If it's used inside @datagouv/components-next it should be installed as a dependency for @datagouv/components-next, no?

Good question! I'm not sure I get it completely, but looks like a combination of source install here + pnpm install downstream. I'm actually happy if it's not installed downstream for now.
BTW, it could be cleaner/more deterministic to declare echarts as a peerDependencies here. The charts endpoint would still make sense.

I let @nicolaskempf57 decide, but I'm not a big fan of adding an entrypoint… It adds some asymmetries to the package for a random feature (charts).

Not so random, echarts is huge.

@abulte abulte changed the title chore(charts): add dedicated entrypoint chore(charts): make echarts optional May 26, 2026
@abulte
Copy link
Copy Markdown
Contributor Author

abulte commented May 26, 2026

@ThibaudDauce @nicolaskempf57 after digging a bit, the original issue is strictly a local one (yalc installed package does not install echarts). I still think it's useful to make the echarts dep optional (and it fixes the original bug), so I changed the scope of this PR.

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.

2 participants