sadpandajoe commented on code in PR #45198:
URL: https://github.com/apache/superset/pull/45198#discussion_r4318930149
##########
superset-frontend/plugins/plugin-chart-chord/package.json:
##########
@@ -29,7 +29,7 @@
"access": "public"
},
"dependencies": {
- "d3": "^3.5.17",
+ "d3": "^7.9.0",
Review Comment:
Bumping to d3 7 will break the chart renderers in this plugin and the other
three bumped here (country-map, partition, world-map), because their sources
are unchanged and still use the v3 API. Each does `import d3 from 'd3'` (v7 has
no default export), and then calls APIs that no longer exist:
`d3.svg.arc`/`d3.layout.chord`/`d3.svg.chord` in `Chord.ts`,
`d3.geo.path`/`d3.behavior.zoom`/callback-style `d3.json` in `CountryMap.ts`,
and `d3.scale.linear` in `Partition.ts` and `WorldMap.ts` (plus
`d3.mouse`/`d3.event`, which are also gone). Any existing chord, country map,
partition or world map chart would fail to render instead of drawing.
Also, `jest.config.js` only allows a few specific `d3-*` ESM packages
through the transform, so the existing `CountryMap`/`WorldMap` tests would
likely fail at import under the ESM-only v7 entry before reaching their
assertions. And nothing here renders Chord or Partition through the real
renderer, so those two wouldn't catch this.
Is the renderer migration to d3 7 planned as a separate change, and should
this bump wait for it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
