Repository navigation
feat: DH-23610: update plotly.js to v4.1.1 - #2759
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2759 +/- ##
==========================================
+ Coverage 52.84% 52.97% +0.12%
==========================================
Files 816 816
Lines 47388 47388
Branches 12245 12433 +188
==========================================
+ Hits 25044 25104 +60
+ Misses 22324 22264 -60
Partials 20 20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved React and Node compatibility metadata issues remain, along with an unsupported indicator trace type.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Upgrades Plotly.js and React Plotly, adapting chart integration and typings for Plotly v4.
Changes:
- Adds merged Plotly trace typings and ambient module declarations.
- Updates chart models, utilities, and React integration.
- Removes obsolete type packages and refreshes dependencies.
- Updates the required Node.js version.
File summaries
| File | Summary | Review notes |
|---|---|---|
packages/chart/src/plotly/plotlyTypes.ts |
Defines merged trace types. | IndicatorData is accepted but not registered in the runtime bundle. |
packages/chart/src/plotly/Plotly.ts |
Updates partial bundle imports. | No final comment. |
packages/chart/src/plotly/plotly-extends.d.ts |
Declares Plotly submodules. | No final comment. |
packages/chart/src/plotly/createPlotlyComponent.ts |
Updates the factory import. | No final comment. |
packages/chart/src/MockChartModel.ts |
Adapts mock data types. | No final comment. |
packages/chart/src/index.ts |
Exports PlotData. |
No final comment. |
packages/chart/src/FigureChartModel.ts |
Migrates model data types. | No final comment. |
packages/chart/src/ChartUtils.ts |
Updates Plotly utility typings. | No final comment. |
packages/chart/src/ChartModel.ts |
Updates the getData() contract. |
No final comment. |
packages/chart/src/Chart.tsx |
Updates chart integration and state types. | No final comment. |
packages/chart/package.json |
Upgrades Plotly dependencies. | React peer range and Node engine range are incompatible with the new dependencies. |
package.json |
Removes obsolete type packages. | No final comment. |
package-lock.json |
Refreshes dependency resolutions. | No final comment. |
.nvmrc |
Updates the Node.js version. | No final comment. |
Review details
Suppressed comments (1)
packages/chart/src/plotly/plotlyTypes.ts:18
IndicatorDatais included here, but the custom bundle inPlotly.tsnever imports or registers theindicatormodule. Becauseplotly.js/lib/core.jspre-registers no trace modules, the exportedPlotDatatype accepts indicator traces that this chart cannot render. Register the module or removeIndicatorDatafrom this union so the documented supported-type list matches the runtime bundle.
| IndicatorData
- Files reviewed: 12/14 changed files
- Comments generated: 2
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved dependency metadata, published declaration, trace registration, and compatibility issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
packages/chart/package.json:46
- This changes the published
@deephaven/chartpeer contract from React>=16.8.0to only React 18/19, so React 16/17 consumers can no longer satisfy the package even though the previous contract allowed them. Please include this compatibility break in the PR'sBREAKING CHANGEdescription; it currently documents only thegetData()and type-package changes.
"react": "^18.0.0 || ^19.0.0"
packages/chart/src/plotly/plotlyTypes.ts:18
SupportedTraceDatais documented as the trace types registered by the partial bundle, butIndicatorDatais included here whilePlotly.tsnever imports or registersplotly.js/lib/indicator.js. This lets consumers create a type-validindicatortrace that the bundled runtime cannot render; either register the module or remove this type from the supported union.
| IndicatorData
- Files reviewed: 12/14 changed files
- Comments generated: 2
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
There was a problem hiding this comment.
🟡 Changes recommended
The public trace type advertises unsupported traces, and the breaking compatibility changes are incompletely documented.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packages/chart/src/plotly/plotlyTypes.ts:52
- The new public
PlotDatadoes not match the partial bundle it describes:SupportedTraceDataincludesIndicatorDataeven thoughPlotly.tsnever registers the indicator module, and wideningtypetoPlotTypeadmits every other unregistered trace as well. Consumers can therefore construct data that type-checks against@deephaven/chartbut fails when rendered by its exported partialPlot; narrow this API to the registered trace literals (or register every advertised trace), and keep arbitrary server input in a separate type.
- Files reviewed: 18/20 changed files
- Comments generated: 2
- Review effort level: Balanced
Update plotly.js 3.1.0 -> 4.1.1 and react-plotly.js ^2.6.0 -> ^4.1.0. Both packages now ship their own types, so @types/plotly.js and @types/react-plotly.js are dropped. plotly.js v4 replaced the catch-all `PlotData` type with one interface per trace type, so `plotly/plotlyTypes.ts` merges the trace types our partial bundle registers back into a single `PlotData`. The untyped `plotly.js/lib/*` subpaths and `react-plotly.js/factory` are declared ambiently, which allows the `@ts-ignore` comments in `plotly/Plotly.ts` to be removed. Bump .nvmrc to v24.20.0 for the npm version required to install. BREAKING CHANGE: `ChartModel.getData()` now returns `Partial<PlotData>[]` using the `PlotData` type exported from `@deephaven/chart` instead of plotly's `Partial<Data>[]`, and `@deephaven/chart` no longer depends on `@types/plotly.js`.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
plotly.js 4.1.1 declares `engines.node: >=22.0.0`, but `@deephaven/chart` still claimed `>=16`, so the published metadata advertised support for a range that cannot be installed under engine-strict. Bump `@deephaven/chart` and every package that transitively depends on it via `dependencies`: app-utils, console, dashboard-core-plugins, iris-grid, plugin and pouch-storage. iris-grid, plugin and pouch-storage reach chart through `@deephaven/console`. code-studio and embed-widget are also in the closure but declare no engines field. BREAKING CHANGE: these packages now declare `engines.node: >=22`. Node 16-21 consumers were already unable to install plotly.js v4 under engine-strict; the metadata now reflects that.
04ae491 to
49da49a
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The supported trace contract and transitive React peer ranges are inconsistent with the updated dependencies.
Review effort: Balanced
Findings: 2
Open (4)
PlotDatanow advertisesScatterData, but the custom bundle still registers onlyscatterglin… This narrows@deephaven/chartto React 18/19, but published dependents such as… Changing the peer range from React 16.8+ to React 18/19 drops React 16 and 17 consumers, which is… Thisengineschange—and the propagated>=22bumps in the dependent published packages—drops…
react-plotly.js v4 peers on `react: ^18.0.0 || ^19.0.0`, and `@deephaven/chart` now declares the same range. Packages that depend on chart still advertised `react >=16.8.0` (or an open-ended `>=18.0.0`), which React 16/17 consumers could not satisfy through the nested peers. Align `react` and `react-dom` peer ranges to `^18.0.0 || ^19.0.0` for console, dashboard-core-plugins, app-utils, iris-grid and pouch-storage. iris-grid and pouch-storage reach chart through `@deephaven/console`. BREAKING CHANGE: `@deephaven/chart`, `@deephaven/console`, `@deephaven/dashboard-core-plugins`, `@deephaven/app-utils`, `@deephaven/iris-grid` and `@deephaven/pouch-storage` now require React 18 or 19 (`react`/`react-dom` `^18.0.0 || ^19.0.0`). React 16/17 consumers must upgrade React before upgrading these packages.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The breaking-change notes omit the newly dropped Node and React versions.
Review effort: Balanced
Findings: None
Resolved since last review (4)
PlotDatanow advertisesScatterData, but the custom bundle still registers onlyscatterglin… This narrows@deephaven/chartto React 18/19, but published dependents such as… Changing the peer range from React 16.8+ to React 18/19 drops React 16 and 17 consumers, which is… Thisengineschange—and the propagated>=22bumps in the dependent published packages—drops…


Update plotly.js 3.1.0 -> 4.1.1 and react-plotly.js ^2.6.0 -> ^4.1.0. Both
packages now ship their own types, so @types/plotly.js and
@types/react-plotly.js are dropped.
plotly.js v4 replaced the catch-all
PlotDatatype with one interface pertrace type, so
plotly/plotlyTypes.tsmerges the trace types our partialbundle registers back into a single
PlotData. The untypedplotly.js/lib/*subpaths andreact-plotly.js/factoryare declaredambiently, which allows the
@ts-ignorecomments inplotly/Plotly.tstobe removed.
These are just types breaking changes, there should be no
runtime breaking changes with the
@deephaven/js-plugin-plotly-expressplugin. Ran all plotly-express e2e tests against this UI and they passed.
Bump .nvmrc to v24.20.0 for the npm version required to install.
BREAKING CHANGE:
ChartModel.getData()now returnsPartial<PlotData>[]using the
PlotDatatype exported from@deephaven/chartinstead ofplotly's
Partial<Data>[], and@deephaven/chartno longer depends on@types/plotly.js. These are just types breaking changes, there should be noruntime breaking changes with the
@deephaven/js-plugin-plotly-expressplugin.