Conversation
plotly.js ships its own type declarations since 4.0 and TypeScript prefers those over DefinitelyTyped, so the package was already unused. Import the types from plotly.js explicitly instead of going through the UMD namespace, and add small local types for the graph div members and events plotly.js doesn't declare (_fullData, _fullLayout, plotly_update, plotly_webglcontextlost). mimeExtension.ts used a default import as a namespace, which never resolved. Also switch moduleResolution to "bundler": "node" was removed in TypeScript 7, so `npm run typecheck` couldn't run at all. Closes plotly#5740
Contributor
|
Thanks for the PR! I'll take a look and follow up. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Link to issue
Closes #5740
Description of change
Drops the
@types/plotly.jsdev dependency fromjs/. plotly.js ships its own declarations since 4.0 ("types": "./lib/index.d.ts"), and TypeScript prefers a package's bundled types over DefinitelyTyped, so the DT package was already unused:tscresolvedplotly.jsto the bundled types all along, andPlotly.PlotlyHTMLElementonly kept working through theexport as namespace PlotlyUMD declaration.While at it, the type usage is made explicit against the bundled declarations:
widget.tsimportsPlotlyHTMLElementand uses a smallPlotlyGraphDivtype for what plotly.js doesn't declare:_fullData/_fullLayouton the graph div and theplotly_updateevent. Theplotly_deselect/plotly_doubleclickhandlers take no argument now, matching the declared callback signature (they never used it).mimeExtension.tshadimport type PlotlyType from "plotly.js"and thenPlotlyType.Dataetc. The default export is a value, not a namespace, so those never resolved. It isimport type * as PlotlyTypenow, plus the same kind of local type forplotly_webglcontextlost.tsconfig.json:moduleResolution: "node"(node10) was removed in TypeScript 7, whichpackage.jsonalready pins (^7.0.2), sonpm run typecheckfailed before checking anything. Switched tobundler, which matches how esbuild consumes the sources.Demo
npm run typecheckonmain:Forcing
--moduleResolution bundleronmaingives 27 errors, 9 of them about plotly.js types:Cannot find namespace 'PlotlyType'(4×),No overload matches this callforplotly_update,plotly_deselectandplotly_doubleclick, andProperty '_fullData' / '_fullLayout' does not exist on type 'PlotlyHTMLElement'. After this change it reports 18, all pre-existing and unrelated to plotly.js (implicitany, strict null checks, no declarations forlodash-esand@jupyterlab/rendermime-interfaces). I left those alone.Testing strategy
No Python changes. Rebuilt with
npm ci && npm run buildafter deletingplotly/labextension/: the labextension static bundle andjs/lib/mimeExtension.jscome out byte-identical (only types changed there),plotly/labextension/package.jsonpicks up the dependency removal, andplotly/package_data/widgetbundle.jschanges because of the two handler signatures (the rest of its diff is the minifier renaming identifiers). Type-checked as described above.Additional information (optional)
No changelog entry, since this only touches the JS dev toolchain. Happy to add one if you prefer; otherwise the changelog check needs the
no-changeloglabel.Guidelines