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.

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

Node 20 -> 22 #5097

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Node 20 -> 22 #5097

wants to merge 11 commits into from

Conversation

birkskyum
Copy link
Member

@birkskyum birkskyum commented Nov 22, 2024

22 has been the lts for 7 months now, so i think we're plenty safe to use it.

https://endoflife.date/nodejs

CI breaks because of cache of canvas in node_modules

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@HarelM
Copy link
Collaborator

HarelM commented Nov 22, 2024

I like it that there's a need to change only in one place! Having said that, CI need more tender love and care...

Copy link

codecov bot commented Nov 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.33%. Comparing base (30ab703) to head (689f2fe).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5097      +/-   ##
==========================================
- Coverage   91.81%   91.33%   -0.48%     
==========================================
  Files         279      279              
  Lines       38340    38340              
  Branches     6698     6663      -35     
==========================================
- Hits        35200    35017     -183     
- Misses       3007     3178     +171     
- Partials      133      145      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@birkskyum
Copy link
Member Author

birkskyum commented Nov 25, 2024

@HarelM , CI is a lot better now.

Basically, 2 things.

  1. The "canvas" package was causing trouble with installation. I see this a lot. They are solving it with their v3, of which I here use a pre-release, which doesn't require manual rebuilds of the package on isntallation. It's being used only for the Offscreen Canvas tests below:

describe('util readImageDataUsingOffscreenCanvas', () => {
test('reads pixels from image', async () => {
(window as any).OffscreenCanvas = Canvas;
const image = new Canvas(2, 2);
const context = image.getContext('2d');

test('normal operation does not mangle canvas', () => {
const OffscreenCanvas = (window as any).OffscreenCanvas = vi.fn((width:number, height: number) => {
return new Canvas(width, height);
});
expect(offscreenCanvasSupported()).toBeTruthy();
OffscreenCanvas.mockClear();
expect(isOffscreenCanvasDistorted()).toBeFalsy();
expect(OffscreenCanvas).toHaveBeenCalledTimes(1);
});

  1. npm began complaining a lot here, leaving me only two options; add the --legacy-peer-deps flags, or move to a more modern package manager like pnpm that wouldn't have a problem in the first place.

@birkskyum birkskyum requested a review from HarelM November 25, 2024 12:42
@HarelM
Copy link
Collaborator

HarelM commented Nov 25, 2024

Can we replace the canvas package with a vitest package somehow? I don't like using non-official packages...

Also, what's the error you get that needs legacy peer deps flag?

@birkskyum
Copy link
Member Author

birkskyum commented Nov 25, 2024

The error is that JSDOM has a peer dependency on "canvas": 2.x, and this upgrades to canvas 3.x. Canvas 3.x is breaking in the sense that it dropped support for Node 16, but JSDOM will still work perfectly fine with it.

.nvmrc Outdated Show resolved Hide resolved
@HarelM
Copy link
Collaborator

HarelM commented Nov 25, 2024

Is there a JSDom version that "is compatible" with canvas 3? Maybe a pre-release of JSDom?
Otherwise I would consider waiting for official releases of these packages...

@@ -1,2 +1,3 @@
engine-strict = true
lockfileVersion = 3
legacy-peer-deps = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be removed when using skia-canvas?

@HarelM
Copy link
Collaborator

HarelM commented Dec 12, 2024

@birkskyum what's the status for this PR? Can we push it to finalization?

@birkskyum
Copy link
Member Author

birkskyum commented Dec 12, 2024

Status is that the JSDOM team prefer to move forward only when a stable v3 of node-canvas is out.

The 3.0.0-rc2 came out in june, so it appeared to progress quite slowly, but interestingly the rc3 came out only 5 days ago.

And there are indications it could move forward any day now.

(maintainer) I've got some time carved out this weekend to try to get 3.0 released, though.

@HarelM
Copy link
Collaborator

HarelM commented Dec 24, 2024

Looks like canvas 3.0 was released.
The PR is failing (due to jsdom dependency I guess), but maybe there a way forward now with this PR:

@birkskyum
Copy link
Member Author

The Peer deps are being updated in Jsdom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants