Skip to content
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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Update optional peer dependency canvas to v3 #3806

Merged
merged 3 commits into from
Jan 9, 2025
Merged

Update optional peer dependency canvas to v3 #3806

merged 3 commits into from
Jan 9, 2025

Conversation

sebastianwachter
Copy link
Contributor

@sebastianwachter sebastianwachter commented Dec 24, 2024 •
edited
Loading

Canvas v3.0.0 is now out of Release Candidate.

Should canvas v2.x.x compatibility be removed now that a stable version is available for newer node versions? (removed)

@birkskyum
Copy link
Contributor

Not release candidate anymore - canvas 3 is out

@domenic
Copy link
Member

domenic commented Dec 24, 2024

Yes, please remove canvas v2 from the peer dependencies and testing matrix so that we don't have the extra support burden!

@alumni alumni mentioned this pull request Dec 24, 2024
Closed
@birkskyum birkskyum mentioned this pull request Dec 24, 2024
Merged
8 tasks
@sebastianwachter
Copy link
Contributor Author

I removed canvas v2 from the test matrix (I removed the test matrix) and from the peer dependencies. I set node version for that pipeline step to v22.

alumni
alumni reviewed Dec 25, 2024
View reviewed changes
.github/workflows/jsdom-ci.yml Outdated
- name: Run tests
run: npm test -- --retries 1
run: npm i canvas && npm test -- --retries 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that the test setup is not that great (probably there should be a test project with both canvas and jsdom) but:

  1. Installing dependencies should not be done in 2 steps. canvas should be installed together with the other dependencies. This will also keep the test output clean.
  2. Not specifying a version when installing a package will always install the latest. This will install canvas@4.0.0 when it will be released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm currently on my way to a ski trip and will implement the changes this evening.

I have a question regarding your second point: will npm i canvas always install the latest version even when the field "peerDependencies": { "canvas": "^3.0.0" } is set? I thought (or hoped) it would install the latest minor or patch version of the v3 major version.

Copy link
Contributor

Choose a reason for hiding this comment

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

npm i canvas will modify package.json and the lockfile and will add the latest version of canvas to the dependencies group. I'm not sure if npm will also remove it from peerDependencies (might also depend on the npm version), some package managers do this.

Viel Spaß! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the npm i canvas@3 to the same line as npm ci and specified canvas in its installtion with @3

@birkskyum
Copy link
Contributor

birkskyum commented Jan 1, 2025 •
edited
Loading

Can we land this, so that jsdom will support node 22? The changes by @sebastianwachter seems to avoid the issue with an eventual canvas v4

@alumni
Copy link
Contributor

alumni commented Jan 2, 2025 •
edited
Loading

Can we land this, so that jsdom will support node 22?

There is no change in API between canvas@2.11.2 and canvas@3.0.1, so you can use them interchangeably.

So unless you enable npm/strict-peer-deps or pnpm/strict-peer-dependencies, you should be able to provide canvas@3 and jsdom would use it without issues.

@sebastianwachter
Copy link
Contributor Author

The thing causing issues is that canvas v3 is not listed in jsdom's peer dependencies which can cause issues like this one (dependabot wants to update to canvas v3 but can't): https://github.com/sebastianwachter/wc-scratch/actions/runs/12568528588/job/35036163215?pr=212

@alumni
Copy link
Contributor

alumni commented Jan 2, 2025 •
edited
Loading

We probably need to wait for @domenic to merge this PR and release a new version.

The thing causing issues is that canvas v3 is not listed in jsdom's peer dependencies

As mentioned above, you should not have this issue unless you explicitly set strict-peer-deps to true (see the link).

For me, Renovate and pnpm install work just fine with the defaults.

@marklundin marklundin mentioned this pull request Jan 7, 2025
Closed
@keithchew
Copy link

I can also confirm npm install fails:

npm ERR! code ERESOLVE
npm ERR! ERESOLVE could not resolve
npm ERR!
npm ERR! While resolving: jsdom@25.0.1
npm ERR! Found: canvas@3.0.1
npm ERR! node_modules/canvas
npm ERR!   canvas@"^3.0.1" from the root project
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peerOptional canvas@"^2.11.2" from jsdom@25.0.1
npm ERR! node_modules/jsdom
npm ERR!   jsdom@"^25.0.1" from the root project
npm ERR!
npm ERR! Conflicting peer dependency: canvas@2.11.2
npm ERR! node_modules/canvas
npm ERR!   peerOptional canvas@"^2.11.2" from jsdom@25.0.1
npm ERR!   node_modules/jsdom
npm ERR!     jsdom@"^25.0.1" from the root project
npm ERR!
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.

@marklundin
Copy link

This would be super helpful. We're currently waiting on an 3.0.1 canvas which fixes an issue with Apple Silicon, and currently we can't avoid the strict-peer-deps

domenic
domenic approved these changes Jan 9, 2025
View reviewed changes
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Thanks!

@domenic domenic merged commit 7cc3500 into jsdom:main Jan 9, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alumni alumni alumni left review comments

@domenic domenic domenic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sebastianwachter @birkskyum @domenic @alumni @keithchew @marklundin