mozilla / pdf.js

PDF Reader in JavaScript
https://mozilla.github.io/pdf.js/
Apache License 2.0
47.93k stars 9.92k forks source link

Upgrade `canvas` to version 3.0.0 #18049

Open timvandermeij opened 4 months ago

timvandermeij commented 4 months ago

In PR #18009 the GitHub Actions CI workflow has temporary been updated to not use the latest Node.js version (22 as of writing) but Node.js 21 instead because the canvas dependency doesn't provide pre-built binaries for Node 22.js yet. We don't want to build it ourselves because that'd require additional complexity for local and workflow installations, and sadly it looks like this may take some time to resolve upstream given that https://github.com/Automattic/node-canvas/issues/2377 and the corresponding release tracking issue https://github.com/Automattic/node-canvas/issues/2232 didn't have recent updates. To avoid having failing builds in the meantime, we chose to pin Node.js to version 21 for the time being.

If https://github.com/Automattic/node-canvas/issues/2377 is resolved upstream and a new release is published with pre-built binaries, we should update to that new version and revert https://github.com/mozilla/pdf.js/pull/18009/commits/e561a4af3c0d73a3c7e2a1751032e1216950095a so we test against the latest version again.

Snuffleupagus commented 3 months ago

Given that Node.js 21 is no longer maintained, according to https://github.com/nodejs/Release/blob/6209d04302e62156b964a605f619283582334c95/schedule.json#L117-L121, should we stop testing in that environment?

timvandermeij commented 2 months ago

On the one hand I think we should, but on the other hand it's the highest version we have at the moment because switching back to latest is still blocked on node-canvas releasing a new stable version with the fix. Ideally we'd wait for that, but given the slow movement of that issue I think we can consider using the unstable release mentioned in https://github.com/Automattic/node-canvas/issues/2377#issuecomment-2179622259 for the time being. Now that there is an official unstable release I'm more comfortable with that, assuming the required prebuilds are in fact available, because the previously mentioned approach in that thread of using the master branch without a release really didn't seem like a viable solution to me.

What do you think about this?

Snuffleupagus commented 2 months ago

What do you think about this?

I actually tried updating that earlier today, but unfortunately it didn't seem to work on GitHub actions; here's the patch: https://github.com/mozilla/pdf.js/compare/master...Snuffleupagus:pdf.js:node-canvas-3

timvandermeij commented 1 month ago

Testing with Node 21 has been removed in https://github.com/mozilla/pdf.js/pull/18505, so we can only more forward with testing with Node 22 or newer once there is a resolution for the canvas dependency.

GitMurf commented 1 month ago

@timvandermeij would you mind briefly explaining what the implications are here short term? Should we be using node-canvas@next? Is there anything special we should be doing / configuring or keeping an eye out for? Thanks!

timvandermeij commented 1 month ago

For now we keep using node-canvas 2.x together with Node < 22 until node-canvas 3.x is released.

kotasudhakar commented 3 weeks ago

They have now released 3.0 beta @timvandermeij

Snuffleupagus commented 3 weeks ago

They have now released 3.0 beta

There's not been any recent releases, so nothing has changed since https://github.com/mozilla/pdf.js/issues/18049#issuecomment-2181143840 was made. (And please refrain from needlessly tagging users, since that creates notification spam.) The latest pre-release, which is v3.0.0-rc2 as of this writing, unfortunately suffers from a pretty trivial bug that prevents usage; see https://github.com/Automattic/node-canvas/issues/2415

Snuffleupagus commented 5 days ago

The latest pre-release, which is v3.0.0-rc2 as of this writing, unfortunately suffers from a pretty trivial bug that prevents usage; see Automattic/node-canvas#2415

This has been fixed, and installing node-canvas seems to work now; see https://github.com/mozilla/pdf.js/compare/master...Snuffleupagus:pdf.js:node-canvas-3

However, running gulp unittestcli either locally or on GitHub Actions fails with the following output:

[11:34:57] Starting 'runUnitTestCli'...
[11:34:57] 'runUnitTestCli' errored after 131 ms
[11:34:57] Error: Unit tests failed.
    at ChildProcess.<anonymous> (file:///home/runner/work/pdf.js/pdf.js/gulpfile.mjs:1896:14)
    at ChildProcess.emit (node:events:519:28)
    at ChildProcess.emit (node:domain:551:15)
    at maybeClose (node:internal/child_process:1105:16)
    at ChildProcess._handle.onexit (node:internal/child_process:305:5)
    at Process.callbackTrampoline (node:internal/async_hooks:130:17)
[11:34:57] 'unittestcli' errored after 8.13 s