NotificationsYou must be signed in to change notification settings
Fork2k
Star18k

Fix: Fix installing dependencies in node v22#7381

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

archmoj merged 7 commits intoplotly:masterfromchaigh-uk:fix-deps-node-22

Jun 3, 2025

Merged

Fix: Fix installing dependencies in node v22#7381

archmoj merged 7 commits intoplotly:masterfromchaigh-uk:fix-deps-node-22

Jun 3, 2025

Conversation

Copy link

Contributor

chaigh-uk commentedFeb 27, 2025

Fixes#7306

Bumpscanvas tov3.1.0 andjsdom tov26.0.0

The key fix was in canvas and isdetailed here

chaigh-uk added2 commits

February 27, 2025 15:58

Fix installing dependencies in node v22

e91848e

Add draft log

436b1be

gvwilson requested a review fromalexcjohnson

February 28, 2025 17:48

gvwilson added community

community contribution

considered for next cycle

fix

fixes something broken

dependenciesPull requests that update a dependency file labels

Feb 28, 2025

Copy link

Contributor

gvwilson commentedFeb 28, 2025

thanks@chaigh-uk - I'll try to prioritize review in the next cycle.

archmoj mentioned this pull request

Mar 31, 2025

Dependency Installation Fails on Node 22#7306

Closed

archmoj reviewed

Mar 31, 2025

View reviewed changes

package.json

		"bubleify":"^2.0.0",
		"buffer":"^6.0.3",
		"canvas":"^2.11.2",
		"canvas":"^3.1.0",

Copy link

Contributor

Choose a reason for hiding this comment

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

Noting thatcanvas v3 has dropped support for Node.js 16.x and below and "node": ">=18.0.0" is used inplotly.js's current package system

plotly.js/package.json

Line 24 in40f2221

"node":">=18.0.0"

, this change seems good to me.
💯 Thank you 🙏

archmoj reviewed

Mar 31, 2025

View reviewed changes

package.json

		"jasmine":"3.5.0",
		"jasmine-core":"3.5.0",
		"jsdom":"^24.1.1",
		"jsdom":"^26.0.0",

Copy link

Contributor

Choose a reason for hiding this comment

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

jsdom v25 appears to improve security issues in regards to the"dangerously" option used bytasks/util/plotly_node.mjs
This dependency bump from v24 to v26 seems good to me.

Copy link

Contributor

chaigh-uk commentedApr 2, 2025

@chaigh-uk Thanks very much for the PR.
You could fetch upstream/master and then add a test titledpublish-dist-node-v22 to regenerateplot-schema similar topublish-dist.
publish-dist is declared at
plotly.js/.circleci/config.yml
Line 410 in40f2221
publish-dist:
You also need to add your newpublish-dist-node-v22 test below here:
plotly.js/.circleci/config.yml
Line 553 in40f2221
-publish-dist
Thank you!

Thanks for taking a look@archmoj - I've had a go at this in46e0277
Hopefully I've understood correctly, but let me know if I've missed anything 👍

chaigh-uk changed the base branch frommaster topr_7005-test

April 2, 2025 11:27

chaigh-uk changed the base branch frompr_7005-test tomaster

April 2, 2025 11:27

gvwilson added P1needed for current cycle and removed P2considered for next cycle labels

Apr 2, 2025

Copy link

Contributor

archmoj commentedApr 3, 2025

💯 🙏 Many thanks@chaigh-uk for the smart addition of node 22 tests.

It is interesting to see node v22 tests appear to run faster.
The failing baseline tests (listedhttps://app.circleci.com/pipelines/github/plotly/plotly.js/11881/workflows/222c9105-1ddf-48c3-8268-a66cbfc281c3) are not related to this PR.

It would be great if someone else from @plotly/plotly_js also help approve this PR.

On another note@chaigh-uk used a pattern in46e0277 that simplified testing in different container versions.

IMHO - It would be of interest to apply similar pattern in other places in plotly.js and plotly.py and dash tests to reduce/avoid duplicated CircleCI config code
cc:@alexcjohnson @antoinerg @samhinshaw