mozilla / pdf.js

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

`canvas` should not be a dependency if possible #15652

Closed tamuratak closed 1 year ago

tamuratak commented 1 year ago

canvas has been added to the dependency, not devDependency, since v3.0.279. I don't think it is a good idea. Because node-canvas prebuilds aren't available for ARM, users with macOS on ARM will request helps from you, which would increase the maintenance burden. We can say the same thing for users with old Linux for which node-canvas prebuilds aren't available.

When GitHub actions supports macOS on ARM, the situation would change.

Configuration:

Snuffleupagus commented 1 year ago

What do you actually propose that we do instead, since the canvas package is necessary when using the PDF.js library in Node.js environments and we support browsers/Node.js in the same builds?

tamuratak commented 1 year ago

One of options would be moving canvas into optionalDependencies from dependencies. Then, npm i pdfjs-dist will install canvas as a dependency by default, and npm will not stop installing pdfjs-dist even if installing canvas fails.

Users also can executenpm i --omit=optional pdfjs-dist if they want not to install canvas.

Snuffleupagus commented 1 year ago

So by using optionalDependencies we basically change an install-time error into a potential run-time error for some (hopefully small number of) Node.js users, while avoiding any issues for browser users? If so, I suppose that we can try that; please submit a patch.

sunknudsen commented 1 year ago

@tamuratak Thanks for raising flag… just hit use case on M1 Mac.

@Snuffleupagus, will #15655 makes it’s way to pdfjs-dist?

timvandermeij commented 1 year ago

Fixed in release 3.1.81.

nikitakot commented 1 year ago

The npm i --omit=optional pdfjs-dist doesn't seem to work. The canvas dependency is still installed and appears in node_modules and package-lock.json. I use electon.js and electron-rebuild to build my electron app and electron-rebuild tries to rebuild the canvas from the node modules which I don't event need.

silverwind commented 1 year ago

canvas module does not work inside worker threads, which are in use by testing frameworks like vitest.

Can it be dropped or be made optional so the consumer can provide it to pdf.js on-demand? At minimum, the require for it should be wrapped in a try-catch clause.

bn3t commented 1 year ago

For me it worked when doing:

npm i --omit=optional pdfjs-dist@3.5.141

Since, my issue was specifically when running unit tests combining vitest and jsdom, I also did:

npm i --omit=optional jsdom

silverwind commented 1 year ago

omit works (best set in .npmrc), but it's dangerous because it will omit all optional dependencies from all your dependencies, not just pdfjs-dist.

bn3t commented 1 year ago

At the end, I have to come back at what I've written in an above comment. --omit=optional didn't work as other dependencies were marked as optional (esbuild-*) but were necessary anyway. I am using another solution which also looks like a hack but seems to work. It comes from this SO answer: https://stackoverflow.com/a/73846274. It consists in adding this to package.json:

"overrides": {
    "canvas": "../_EXCLUDED_"
  }

Since canvas, is an optional dependency, an override to a non existing location is ignored silently.

lucasgadams commented 1 year ago

Is there any way to reduce the needed size of the canvas package? when I use pdfjs in a nextjs environment, the canvas package is like 48mb and the limit for Vercel is 50mb! This makes it incredibly difficult to build an app. No other package is even close to this size. Kind of pulling my hair out over this, i currently cant deploy my app. Any ideas?

MattyBalaam commented 1 year ago

I found a workaround to stop installing canvas if that might help? We add a new entry to the resolutions object in package.json for canvas:

https://github.com/mozilla/pdf.js/issues/16463

net commented 1 year ago

I wonder if using @napi-rs/canvas instead would be a good solution for both M1 and Vercel environments.

jakepgoldman commented 1 year ago

@net how would you go about replacing the canvas dep with the package you listed above?