build: Enable TypeScript type checking#7680
Conversation
| var constants = require('./constants'); | ||
| var overrideAll = require('../../plot_api/edit_types').overrideAll; | ||
| var sortObjectKeys = require('../../lib/sort_object_keys'); | ||
| var sortObjectKeys = require('../../lib/sort_object_keys').default; |
There was a problem hiding this comment.
@camdecoster Is it worth considering moving away from default exports entirely as part of this transition?
There was a problem hiding this comment.
It only matters when one mixes CJS and ESM. That said, I don't think that we can avoid that for a long time. I suppose we could make just pick a direction to go and standardize. I could go either way, but the esbuild docs make it clear that they don't like default exports.
| fillcolor?: string; | ||
| line?: Partial<Line>; | ||
| marker?: Partial<Marker>; | ||
| mode?: 'lines' | 'markers' | 'lines+markers' | 'none' | 'text' | 'lines+text' | 'markers+text' | 'lines+markers+text'; |
There was a problem hiding this comment.
Does TypeScript have any support for 'flag list'-type string values such as this, where the string may consist of any number of a fixed set of values joined by a delimiter? I suppose not as it's fairly custom.
Otherwise could we use a custom type or a custom function to generate these lists of allowed values based on the flags?
There was a problem hiding this comment.
Yes, it's called a union type. What's shown on 133 is an example of that (though it's only used on that line). If we needed that type elsewhere, we could define it separately and reference it within the ScatterTrace type like this:
type Mode = 'lines' | 'markers' | 'lines+markers' | 'none' | 'text' | 'lines+text' | 'markers+text' | 'lines+markers+text';
export interface ScatterTrace extends TraceBase {
...,
mode: Mode;
...,
}There was a problem hiding this comment.
Yeah I guess I'm imagining something like a helper function which looks like flagList(flaglistVals, otherVals) such that
flaglistVals(['lines', 'markers', 'text'], ['none'])
returns
'lines' | 'markers' | 'lines+markers' | 'none' | 'text' | 'lines+text' | 'markers+text' | 'lines+markers+text';`
| * Use specific trace types when available | ||
| */ | ||
| export interface GenericTrace extends TraceBase { | ||
| x?: any[]; |
There was a problem hiding this comment.
I'm not sure there are any traces where x/y/z are allowed to be a type other than number[] | string[] but I could be wrong about that.
There was a problem hiding this comment.
That's why it's permissive here. Once we become sure, we can change this as you suggest.
There was a problem hiding this comment.
In this case maybe it would be easier to start stricter and loosen if needed?
|
@camdecoster Can you add a type-check step to the CI? |
| yaxis?: Partial<LayoutAxis>; | ||
|
|
||
| // Multiple axes support (xaxis2, yaxis3, etc.) | ||
| [key: string]: any; |
There was a problem hiding this comment.
Can we tighten this layout spec here to match the plot schema?
|
@camdecoster Some initial thoughts on organization of the types: For types corresponding directly to the schema:
|
codeCraft-Ritik
left a comment
There was a problem hiding this comment.
Great work! The implementation is clean and easy to understand
2439dad to
de168ca
Compare
| * Common properties shared by all trace types — convenience interface for | ||
| * gradual migration of internal code that doesn't need the full PlotData shape. | ||
| */ | ||
| export interface TraceBase { |
There was a problem hiding this comment.
What's the relationship between the TraceBase and PlotData types?
There was a problem hiding this comment.
This is a leftover from a previous attempt at adding types. I'll remove it.
| _length?: number; | ||
| _module?: any; | ||
| index?: number; | ||
| [key: string]: any; |
There was a problem hiding this comment.
Is there any logic behind which keys are explicitly listed here?
There was a problem hiding this comment.
These should be universally available internal properties. There could be more that need to be added, so the escape hatch is at the bottom. This will prevent TS errors from blocking the type check. We can add new properties as we come across them during conversion.
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // LayoutAxis (extends Axis for cartesian subplots) |
There was a problem hiding this comment.
small nit, but I'm not sure this comment is accurate since some of these properties seem to exist for non-cartesian axes as well
There was a problem hiding this comment.
You're right! The naming could be better. Ideally, there would be separate types for each axis type. I'll add a task to fix that after this PR.
| // Mapbox types | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| export interface MapboxCenter { |
There was a problem hiding this comment.
All of these Mapbox* types can be changed to Map* right? AFAIK the API is exactly the same.
There was a problem hiding this comment.
Good point. I'll rename them to Map* and add an alias to Mapbox* for backward compatibility.
| xaxis6: Partial<LayoutAxis>; | ||
| xaxis7: Partial<LayoutAxis>; | ||
| xaxis8: Partial<LayoutAxis>; | ||
| xaxis9: Partial<LayoutAxis>; |
There was a problem hiding this comment.
hm I realize this is taken from DefinitelyTyped but what about plots with more than 9 x/y axes? I don't think there's any hard upper limit
There was a problem hiding this comment.
Yeah, there's no limit. I'll update the types to allow any number.
| hoversubplots: 'single' | 'overlaying' | 'axis'; | ||
| calendar: Calendar; | ||
|
|
||
| // Dotted property paths for Plotly.relayout convenience |
There was a problem hiding this comment.
relayout accepts an attribute string (eg. 'xaxis.range[0]'). Updating the type to handle anything string like that would remove much of the type safety. DT includes these commonly used attribute strings to allow for that type of signature while preserving type safety.
| _modeBar?: any; | ||
| _pushmargin?: { [key: string]: any }; | ||
|
|
||
| [key: string]: any; |
There was a problem hiding this comment.
same question here, is there any reasoning behind which keys are called out explicitly?
There was a problem hiding this comment.
Those were the properties found in the first search pass. More can be added as we convert files. I'll see if I can get a few more added.
| /** | ||
| * Template figure type | ||
| */ | ||
| export interface TemplateFigure { |
There was a problem hiding this comment.
What's the difference between Template and TemplateFigure?
There was a problem hiding this comment.
Templatedescribes a config object that lists settings for different traces. You can define default styling, etc. per trace type.TemplateFiguredescribes an actual figure that can be rendered. Think of what you might include in a devtools mock.
There was a problem hiding this comment.
Shouldn't this be generated from src/components/colorbar/attributes.js?
There was a problem hiding this comment.
It will be eventually. For now, most of the types have been manually added.
There was a problem hiding this comment.
Doesn't this file duplicate the info in src/traces/box/attributes.js to some extent?
There was a problem hiding this comment.
It does for now because that file hasn't been converted to TS yet. When it is, some of all of this info will be generated from the attributes file.
|
|
||
| | User-facing | Internal | Where defined | | ||
| |---|---|---| | ||
| | `Layout` | `FullLayout` | `Layout` is generated; `FullLayout extends Layout` is hand-written | |
There was a problem hiding this comment.
`Layout` is generated; `FullLayout extends Layout` is hand-written
This isn't currently the case based on the contents of this PR though, right? Layout seems to be hand-written.
There was a problem hiding this comment.
You're right. That's an aspirational statement. I'll update it to reflect the current status.
|
|
||
| ```ts | ||
| /** | ||
| * @generates ModeBar |
There was a problem hiding this comment.
Should probably add some guidelines for choosing this canonical public name, since it may not always be obvious.
There was a problem hiding this comment.
I added more info in the "Three things to notice" section below that covers this.
|
|
||
| ### 6. Replace the hand-written type in `src/types/` | ||
|
|
||
| Find the corresponding hand-written type (likely in |
There was a problem hiding this comment.
Will it always be obvious which hand-written type corresponds to the attributes file? How do you determine for sure whether one already exists or not?
There was a problem hiding this comment.
It should be obvious. If it's not, you could search in the types folder for some specific attributes and find it that way.
| If the hand-written type was richer than the schema (e.g. used a narrowed | ||
| union where the schema says `string`), document the gap in a comment or | ||
| file an issue. Do not silently lose ergonomics — either improve the schema | ||
| (add `values: [...]`) or layer a hand-written refinement on top. |
There was a problem hiding this comment.
or layer a hand-written refinement on top
I'm not sure there's really a clean way to do this using the proposed architecture
There was a problem hiding this comment.
You could extend the generated type with the additional information without too much trouble. The clunky part would be the naming. For ModeBar, you could generate ModeBarGenerated then extend it as ModeBar. Not ideal, but it would work.
Description
Enable TypeScript type checking.
Closes #7678.
Changes
Testing
npm run buildlocally to check that everything is bundled properly by esbuildnpm run typecheckto perform a type check on the source code. There should be no errors:Notes
npm run typecheck. VS Code will also provide feedback as you're editing files.TS is not emitting files/types right now. esbuild handles converting the TS so the TS compiler won't need to do that. It would be valuable to have the compiler write the type definition files in the future.TS doesn't emit types, but there's a type generation step that uses TS to create types from attributes files.