Repository navigation
[plotly.js] fix types for image export - #75624
typescript-automation[bot] merged 2 commits into
Conversation
|
@fujidana Thank you for submitting this PR! This is a live comment that I will keep updated. 1 package in this PR
Code ReviewsBecause you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it. You can test the changes of this PR in the Playground. Status
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 75624,
"author": "fujidana",
"headCommitOid": "6f2b44506ef5e4e02bd6b0824db1c81dabbd17f3",
"mergeBaseOid": "c06b1d776527b5422e8f76f3edf36982fd87e0cc",
"lastPushDate": "2026-09-28T04:21:18.000Z",
"lastActivityDate": "2026-10-07T17:59:58.000Z",
"mergeOfferDate": "2026-10-07T16:39:09.000Z",
"mergeRequestDate": "2026-10-07T17:59:58.000Z",
"mergeRequestUser": "fujidana",
"hasMergeConflict": false,
"isFirstContribution": false,
"tooManyFiles": false,
"hugeChange": false,
"tooManyCommits": false,
"tooManyReviews": false,
"popularityLevel": "Popular",
"pkgInfo": [
{
"name": "plotly.js",
"version": "3.0",
"kind": "edit",
"files": [
{
"path": "types/plotly.js/index.d.ts",
"kind": "definition"
},
{
"path": "types/plotly.js/test/core-tests.ts",
"kind": "test"
},
{
"path": "types/plotly.js/v2/index.d.ts",
"kind": "definition"
},
{
"path": "types/plotly.js/v2/test/core-tests.ts",
"kind": "test"
}
],
"owners": [
"chrisgervang",
"martinduparc",
"frederikaalund",
"taoqf",
"Dadstart",
"szechyjs",
"soorajpudiyadath",
"jonfreedman",
"meganrm",
"milesjos",
"skippercool",
"marnett-git",
"peterblazejewicz",
"brammitch",
"blizzardjessica",
"olegshilov",
"PabloGracia",
"jvgogh",
"jpabdou",
"mrtnbrst"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "brammitch",
"date": "2026-10-07T16:38:29.000Z",
"isMaintainer": false
}
],
"mainBotCommentID": 5863296047,
"ciResult": "pass"
} |
|
🔔 @chrisgervang @martinduparc @frederikaalund @taoqf @Dadstart @szechyjs @soorajpudiyadath @jonfreedman @meganrm @milesjos @SkipperCool @marnett-git @peterblazejewicz @brammitch @blizzardjessica @olegshilov @PabloGracia @jvgogh @jpabdou @mrtnbrst — please review this PR in the next few days. Be sure to explicitly select |
|
@fujidana: Everything looks good here. I am ready to merge this PR (at 6f2b445) on your behalf whenever you think it's ready. If you'd like that to happen, please post a comment saying:
and I'll merge this PR almost instantly. Thanks for helping out! ❤️ (@chrisgervang, @martinduparc, @frederikaalund, @taoqf, @Dadstart, @szechyjs, @soorajpudiyadath, @jonfreedman, @meganrm, @milesjos, @SkipperCool, @marnett-git, @peterblazejewicz, @brammitch, @blizzardjessica, @olegshilov, @PabloGracia, @jvgogh, @jpabdou, @mrtnbrst: you can do this too.) |
|
Ready to merge |
f0f4447
into
DefinitelyTyped:master
Please fill in this template.
pnpm test <package to test>.Select one of these and delete the others:
If changing an existing definition:
package.json.This PR fixes the following type definition problems around Plotly.js's image export features:
"full-json"literal type inToImageFormatunion type.setBackgroundproperty inToImgoptsandDownloadImgoptsinterfacesimageDataOnlyproperty inToImgoptsinterface.ToImgoptsandDownloadImgoptswrongly set as required.downloadImage()wrongly set as required.Actually
full-jsonformat option has been available since plotly.js v1.53.0 released 5 years ago (see also plotly/plotly.js#4593) but until recently it was not documented (see plotly/graphing-library-docs#471). Documentation aboutimageDataOnlywas also updated then but it seems the feature has been also available from old versions. Now the updated documentation is available in the official Function Reference and Configuration Options documentation pages.I checked
full-jsonandimageDataOnlychange the exported data as expected on both Plotly.js v2.25.2 and v3.7.0. Also, I confirmed that the properties inToImgoptsandDownloadImgoptsare actually optional (without providing them, no error is thrown) on both versions. I can't find documentation aboutsetBackgroundand don't know when it was added. Anyway, type definitions of this property will not be harmful for existing code since it is optional.Plotly.js v4 started to provide the type definitions by themselves. Reflecting the doc updates mentioned above, theier type definition was also updated (plotly/plotly.js#8066). What this PR updates are semantically very close to what their PR does. Plotly.js v4 currently only bundles the types with the main package. Therefore, for users of a partial package such as
plotly-basic-dist.minthe type definition here is still important. So is for version 3 users, of course.In the process of fixing the problems listed above, the following code changes have been made:
SetBackgroundandToImageFormattypes andToImageButtonOptionsinterface, instead of directly defining them in fields of other types/interfaces.ToImgoptsandDownloadImgoptsfromToImageButtonOptions, instead of redundantly defining their properties.