GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — build: Enable TypeScript type checking by camdecoster · Pull Request #7680 · plotly/plotly.js

Conversation

@camdecoster

@camdecoster camdecoster commented Jan 2, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Description

Enable TypeScript type checking.

Part of #7678.

Changes

  • Add set of plotly.js types
  • Enable type checking
  • Update some files to TypeScript
  • Use ts-node for running some scripts
  • Add type generation script that uses attributes files schema
  • Add type checking steps to CI

Testing

  • CI should cover everything
  • Run npm run build locally to check that everything is bundled properly by esbuild
  • Run npm run schema to recreate the generated types
  • Run plotly.js devtools and load some mocks to make sure everything is working correctly
  • Run npm run typecheck to perform a type check on the source code. There should be no errors: 
    $ npm run typecheck

> [email protected] typecheck
> tsc --noEmit
$

Notes

  • This PR adds the TypeScript compiler and typechecking via npm script
  • The provided types are a combination of plotly.js types from DefinitelyTyped and the schema (with Claude combining the two). This is a first pass and the types will need to be updated as files are converted.
  • Most of the types are now built straight from the schema
  • (Most) IDEs will read the types and provide autocompletion once files are converted over
  • Read the README in src/types for an overview of how the types are set up
  • esbuild is compatible with TypeScript already, but it doesn't do type checking. To check types you need to run npm run typecheck. VS Code will also provide feedback as you're editing files.
  • I converted a few simple files to TypeScript as an example. I opted to use ESM syntax for these files which necessitated some changes in the require statements for the converted files. This is due to how esbuild determines if a default import should be namespaced or not.
  • Some of the converted files are used in scripts not processed by esbuild. Node 18 doesn't handle TS files natively, so these scripts need to be run with ts-node, a "TypeScript execution and REPL for node.js".
  • The TS config is pretty lenient right now to make it easier to convert things piece by piece. Once we make enough progress, this can be tightened up.
  • 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.
  • There's an entry point for types now. It points to the public types, which are a subset of all types. In the future, there could be a type build step that compiles them down to a flat file.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
YakovL, lwolle, and StavNoyAkur8 reacted with heart emoji
All reactions
  • ❤️ 3 reactions
@camdecoster camdecoster changed the title build: Enable typescript type checking build: Enable TypeScript type checking Jan 2, 2026
@camdecoster
camdecoster marked this pull request as ready for review January 2, 2026 23:54
@camdecoster
camdecoster requested a review from emilykl January 7, 2026 16:57
@camdecoster camdecoster assigned emilykl Jan 7, 2026
emilykl
emilykl reviewed Jan 12, 2026
View reviewed changes
Comment thread src/plots/geo/layout_attributes.js
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;

@emilykl emilykl Jan 12, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@camdecoster Is it worth considering moving away from default exports entirely as part of this transition?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@camdecoster camdecoster Jan 12, 2026

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@camdecoster camdecoster May 8, 2026

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an issue that covers converting from CJS to ESM in #7119. I left a comment mentioning your suggestion.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
emilykl
emilykl reviewed Jan 13, 2026
View reviewed changes
Comment thread src/types/core/data.d.ts Outdated
fillcolor?: string;
line?: Partial<Line>;
marker?: Partial<Marker>;
mode?: 'lines' | 'markers' | 'lines+markers' | 'none' | 'text' | 'lines+text' | 'markers+text' | 'lines+markers+text';

@emilykl emilykl Jan 13, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@camdecoster camdecoster Jan 13, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
    ...,
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
emilykl reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@emilykl emilykl Jan 15, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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';`

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
emilykl
emilykl reviewed Jan 13, 2026
View reviewed changes
Comment thread src/types/core/data.d.ts Outdated
* Use specific trace types when available
*/
export interface GenericTrace extends TraceBase {
x?: any[];

@emilykl emilykl Jan 13, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@camdecoster camdecoster Jan 13, 2026

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why it's permissive here. Once we become sure, we can change this as you suggest.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@emilykl emilykl Jan 15, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case maybe it would be easier to start stricter and loosen if needed?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@emilykl emilykl Jan 15, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, scratch that

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@emilykl

emilykl commented Jan 15, 2026

Copy link
Copy Markdown
Contributor

@camdecoster Can you add a type-check step to the CI?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
emilykl
emilykl reviewed Jan 15, 2026
View reviewed changes
Comment thread src/types/core/layout.d.ts Outdated
yaxis?: Partial<LayoutAxis>;

// Multiple axes support (xaxis2, yaxis3, etc.)
[key: string]: any;

@emilykl emilykl Jan 15, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we tighten this layout spec here to match the plot schema?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@emilykl

emilykl commented Jan 15, 2026

Copy link
Copy Markdown
Contributor

@camdecoster Some initial thoughts on organization of the types:

For types corresponding directly to the schema:

  • Group them all under a single directory (e.g. types/core/schema/), maybe even in a single file if not too unwieldy
  • Use nested types where useful for repeated patterns (e.g. font options); these should use some kind of consistent naming scheme
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@archmoj archmoj mentioned this pull request Jan 27, 2026
Set default layout.axis.tickmode to 'sync' when axis is overlaying #7684
Merged
codeCraft-Ritik
codeCraft-Ritik reviewed Feb 28, 2026
View reviewed changes

@codeCraft-Ritik codeCraft-Ritik left a comment

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! The implementation is clean and easy to understand

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@camdecoster camdecoster assigned camdecoster and unassigned emilykl Apr 15, 2026
camdecoster added 9 commits May 5, 2026 16:28
@camdecoster
Linting/formatting
51001fd
@camdecoster
Initial TS set up
cef04d2
@camdecoster
Convert a few small files to TS
412d4b1
@camdecoster
Update requires to handle esbuild ESM/CJS interoperability
f704542
@camdecoster
Use ts-node for scripts using TS that run outside of esbuild
5f72cf8
@camdecoster
Run test script with ts-node
5ed7da0
@camdecoster
Add additional types and type generation script
f2622c5
@camdecoster
Convert modebar attributes to TS
a131cfe
@camdecoster
Add type generation npm scripts
de168ca
@camdecoster
camdecoster force-pushed the cam/7678/enable-typescript-type-checking branch from 2439dad to de168ca Compare May 6, 2026 22:07
camdecoster added 5 commits May 6, 2026 16:13
@camdecoster
Add type check steps to CI
5f8654c
@camdecoster
Update TS docs
eebf71e
@camdecoster
Add entry point to types
c11bca1
@camdecoster
Update file name case check
04261b2
@camdecoster
Linting/formatting
e69463a
@camdecoster
camdecoster requested a review from emilykl May 7, 2026 16:07
emilykl
emilykl reviewed Jun 4, 2026
View reviewed changes
Comment thread src/types/SETUP.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
emilykl
emilykl reviewed Jun 4, 2026
View reviewed changes
Comment thread src/types/SETUP.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
emilykl
emilykl reviewed Jun 4, 2026
View reviewed changes
Comment thread src/types/SETUP.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
emilykl
emilykl reviewed Jun 4, 2026
View reviewed changes
Comment thread src/types/SETUP.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
emilykl
emilykl reviewed Jun 4, 2026
View reviewed changes
Comment thread src/types/SETUP.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
emilykl
emilykl reviewed Jun 4, 2026
View reviewed changes
Comment thread tasks/schema.mjs Resolved

Uh oh!

There was an error while loading. Please reload this page.
emilykl
emilykl reviewed Jun 4, 2026
View reviewed changes
Comment thread tsconfig.json Resolved

Uh oh!

There was an error while loading. Please reload this page.
emilykl
emilykl reviewed Jun 4, 2026
View reviewed changes
Comment thread tsconfig.json Outdated
"declaration": false,
"declarationMap": false,
"sourceMap": true,
"outDir": "./dist",

@emilykl emilykl Jun 4, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed if there's no output?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@emilykl emilykl Jun 4, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh never mind, I guess this config file is used by esbuild as well?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@camdecoster camdecoster Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used by esbuild, but there are some options (including this one) that don't get used. I'll remove them.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
emilykl reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
emilykl
emilykl reviewed Jun 4, 2026
View reviewed changes
Comment thread tasks/generate_schema_types.mjs Outdated
}

// PlotType — derived from the list of trace names, not from an attribute.
found.set('PlotType', { values: Object.keys(schema.traces).sort() });

@emilykl emilykl Jun 4, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really should be called TraceType... the question is, is it worth breaking correspondence with DefinitelyTyped to use the semantically correct type name?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@camdecoster camdecoster Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do whatever we want. I'm less interested in maintaining the DT API since I started using the schema as the source of truth. I'll look into the possibility of changing the name.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
emilykl reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@emilykl

emilykl commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

@camdecoster I think it would be useful to add guidance for converting non-attributes files as well, or at least guidance on which files are the highest-priority to convert and which ones don't need to be touched. But again that doesn't need to be blocking for merge.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
emilykl
emilykl previously approved these changes Jun 4, 2026
View reviewed changes

@emilykl emilykl left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a bunch of nitpicky comments but big picture LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@camdecoster @emilykl
Apply suggestions from code review
0593acf 
Co-authored-by: Emily KL <[email protected]>
@camdecoster
camdecoster dismissed emilykl’s stale review via 0593acf June 5, 2026 02:53
camdecoster added 7 commits June 5, 2026 09:08
@camdecoster
Update docs
d26e84c
@camdecoster
Remove dead options from tsconfig
085069c
@camdecoster
Update JSDoc descriptions
d8028ee
@camdecoster
Tighten up a few types
044e900
@camdecoster
Minor refactoring
87b6e0e
@camdecoster
Loosen data_array type and update schema types
b4dea80
@camdecoster
Rename PlotType to TraceType, add alias
606ae93
@camdecoster camdecoster added this to the v4.0.0 milestone Jun 8, 2026
@camdecoster
camdecoster changed the base branch from master to v4.0 June 8, 2026 15:37
camdecoster added 3 commits June 8, 2026 10:08
@camdecoster
Fix typo
41b43f0
@camdecoster
Merge remote-tracking branch 'origin/v4.0' into cam/7678/enable-types…
2e5e84c 
…cript-type-checking
@camdecoster
Update type docs/generator/schema per removal of Chart Studio code
66f4268
emilykl
emilykl approved these changes Jun 8, 2026
View reviewed changes
@camdecoster
camdecoster merged commit 6031ecb into v4.0 Jun 8, 2026
83 checks passed

Uh oh!

There was an error while loading. Please reload this page.
@camdecoster
camdecoster deleted the cam/7678/enable-typescript-type-checking branch June 8, 2026 18:46
@slaweet slaweet mentioned this pull request Aug 28, 2026
[BUG]: v4.0.0 publishes uncompiled .ts sources that are required extensionless — package fails to load under Node's CJS resolver #7995
Closed
@emilykl emilykl mentioned this pull request Sep 14, 2026
Remove @types/plotly.js dependency plotly/plotly.py#5740
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@codeCraft-Ritik codeCraft-Ritik codeCraft-Ritik left review comments
@emilykl emilykl emilykl approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@camdecoster camdecoster

Labels

None yet

Projects

None yet

Milestone

v4.0.0

Development

Successfully merging this pull request may close these issues.

[FEATURE]: Add TypeScript type checking

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@camdecoster @emilykl @codeCraft-Ritik
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.