Repository navigation
chore: Add TS types for lifecycle and WebGL context events - #8145
camdecoster wants to merge 5 commits into
Conversation
| export type TreemapPlotDatum = SunburstPlotDatum; | ||
|
|
||
| /** Point payload for `plotly_icicleclick`. Same shape as `SunburstPlotDatum`. */ | ||
| export type IciclePlotDatum = SunburstPlotDatum; |
There was a problem hiding this comment.
These types reuse SunburstPlotDatum which declares that value, id, color, and hovertext are all required.
However, at runtime a click payload does NOT always have those fields:
gd.on('plotly_treemapclick', (e) => {
const pt = e.points[0];
panel.textContent = `${pt.label}: ${pt.value.toLocaleString()} people`; // TypeError: pt.value is undefined
loadDetails(pt.id); // undefined without `ids`
});Also, the sunburst event has the same gap today, and this PR extends the gap to two more events.
In this PR, nextLevel became optional for the same reason.
There was a problem hiding this comment.
I'll make these properties optional.
| on( | ||
| event: 'plotly_legendtitleclick' | 'plotly_legendtitledoubleclick', | ||
| callback: (event: LegendTitleClickEvent) => boolean | ||
| ): void; |
There was a problem hiding this comment.
An event handler that only observes the event (i.e. does NOT return false) reports a type error:
gd.on('plotly_legendtitleclick', (e) => {
console.log({ legend: e.legendId });
});error TS2769: No overload matches this call.
The last overload gave the following error.
Argument of type '"plotly_legendtitleclick"' is not assignable to parameter of type '"plotly_afterexport" | ...
There was a problem hiding this comment.
You're right, these should return boolean | void. I'll update them.
| /** Payload for `plotly_react`, emitted after a `Plotly.react` call that left the config unchanged. */ | ||
| export interface PlotReactEvent { | ||
| /** User-supplied config object. */ | ||
| config: Partial<Config>; | ||
| /** User-supplied data array. */ | ||
| data: Data[]; | ||
| /** User-supplied layout object. */ | ||
| layout: Partial<Layout>; | ||
| } |
There was a problem hiding this comment.
Small mismatch here vs what the runtime actually does:
config is not optional here, but a call without a config sends config: undefined.
gd.on('plotly_react', (e) => console.log(e.config.responsive)); // no TS error
Plotly.react(gd, data, layout);
// TypeError: Cannot read properties of undefined (reading 'responsive')There was a problem hiding this comment.
config and layout don't have to be passed in to react, so they are optional here. You can see the logic checking both in the react code.
| export interface WebGLContextLostEvent { | ||
| /** The original DOM `webglcontextlost` event. */ | ||
| event: WebGLContextEvent; |
There was a problem hiding this comment.
This isn't new to this PR, but I just noticed it:
npm run test-types will not catch a broken type reference inside the declarations!
Example:
export interface WebGLContextLostEvent {
/** The original DOM `webglcontextlost` event. */
- event: WebGLContextEvent;
+ event: WebGLContextEvnt;npx tsc --noEmit -p test/types --skipLibCheck falsefails as expectednpm run test-typespasses
It's not obvious, but test/types/tsconfig.json is extending the root tsconfig, which sets skipLibCheck: true.
There was a problem hiding this comment.
Good catch. I'll update the test tsconfig.
Description
Add TypeScript declarations for lifecycle and WebGL context events.
Changes
Testing
npm run test-typesgd.on('plotly_...', (event) => ...)callgd.on('plotly_on a new lineNotes