Image class type in index.d.ts incompatible with HTMLImageElement

[keean]

Issue or Feature

• If this is an issue with installation, I have read the troubleshooting guide.

Writing TypeScript code to run in both browser and backend has been broken with relatively recent changes.

In types/index.d.ts, the following changes are needed:

--- a/node_modules/canvas/types/index.d.ts
+++ b/node_modules/canvas/types/index.d.ts
@@ -324,10 +327,10 @@ export class Image {
 	 * can embed JPEGs directly into the output, rather than re-encoding into
 	 * PNG. This can drastically reduce filesize and speed up rendering.
 	 */
-	dataMode: number
+	dataMode?: number
 
-	onload: (() => void) | null;
-	onerror: ((err: Error) => void) | null;
+	onload: ((this: GlobalEventHandlers, ev: Event) => any) | null;
+	onerror: ((err: string|Event) => void) | null;
 }

dataMode needs to be made optional, as it does not exist in the browser, and the types for onload and onerror are incorrect.

Steps to Reproduce

import { createCanvas } from 'canvas';
const canvas = Canvas.createCanvas(200, 200);
const ctx = canvas.getContext('2d');
const image = new Image();
cxt.drawImage(image, 0, 0, canvas.width, canvas.height);

In the browser, 'new Image()' creates an HTMLImageElement, which cannot be passed to 'drawImage' because the Image type from node-canvas is incompatible.

Your Environment

[email protected]
[email protected]
6.1.8-gentoo-x86_64 #1 SMP PREEMPT_DYNAMIC Fri Jan 27 17:36:19 GMT 2023 x86_64 AMD Ryzen 9 5950X 16-Core Processor AuthenticAMD GNU/Linux

[chearon]

The parts in the green would be wrong, as we always set dataMode, we do not pass an argument to onload, and we never pass a string to onerror.

If it used to build, that was deceptive. node-canvas types are not going to match browser types exactly. You can import the types and have your functions work with either one, though, and that will be less error prone:

/// <reference lib="dom" />
import type {CanvasRenderingContext2d as NodeCanvasRenderingContext2D} from 'canvas';
function drawToContext(ctx: CanvasRenderingContext2d | NodeCanvasRenderingContext2D) {
   // ...
}

I'm not sure what you mean in the "steps to reproduce", because that code does pass type checking. If you have a more full example of code that's supposed to run in both browser and in node I might be able to help better.

[keean]

@chearon So I tried using this as you suggested:

import type {CanvasRenderingContext2d as NodeCanvasRenderingContext2D} from 'canvas';
function drawToContext(ctx: CanvasRenderingContext2d | NodeCanvasRenderingContext2D) {
   // ...
}

However there is a problem in that NodeCanvas supplies "createCanvas" which the. documentation says should work on the frontend or the backend. This returns a Canvas type and calling getContext in this returns a NodeCanvasRenderingContext2D object whether this is running in the browser or the backend.

the return type of getContext on Canvas would seem to need to be modified to return NodeCanvasRenderingContext2D|CanvasRenderingContext2D.

[chearon]

whether this is running in the browser or the backend

I don't know how to interpret this since node-canvas can't be called in the browser. Your nodejs-only and browser-only code should get the context separately and pass to code that works with either browser or node-canvas. Can you post a minimal example?

[chearon]

Oh, I see it now. I didn't know about browser.js. We should probably modify the return type there. Not sure I like that file though...

[keean]

@chearon The original design of NodeCanvas which worked for years is that you could use the NodeCanvas API both in the client and the server. In the client it provided a wrapper around the browser objects, and in the server it provided equivalent functionality.

My application relies on this behaviour and recent changes to NodeCanvas that has broken these assumptions has caused me a lot of problems, that are in fact getting worse with each release of NodeCanvas now.

Having said that, I would settle for anything that just worked at this point, but I have problems with drawImage and putImageData

Where for example "new Image" works in both browser and node, but produces different types, and then we somehow have to deal with two different types of context. It's the same with 'createCanvas'...

Really I just want to know the current 'official' way to do this, so if I fix my code, it won't keep breaking on every release, like it has recently.

[LinusU]

The original design of NodeCanvas which worked for years is that you could use the NodeCanvas API both in the client and the server. In the client it provided a wrapper around the browser objects, and in the server it provided equivalent functionality.

I personally think that this is quite neat! (although I was the one who added it so I'm probably biased 😂)

For the next major version I think that it would be nice if we specifically implemented OffscreenCanvas instead of Canvas. That way creating a new canvas would work the same on both web and Node.js (new OffscreenCanvas(width, height)). ref: https://developer.mozilla.org/en-US/docs/Web/API/OffscreenCanvas

With that said, this is just my personal opinion, and I haven't looked too much in to it to see how feasible it would be...

I have also been working on an Image and ImageData implementation that should behave exactly the same in Node.js as in the browser:

https://github.com/node-gfx/image
https://github.com/node-gfx/image-data

Using these in the next major version of canvas (since the onload will behave as in the browser it would be a breaking change) I think would also be a good thing.

[chearon]

I can see why people like the browser.js entry point and we should try and fix it if we can. But can we? createCanvas would need to return HTMLCanvasElement in the browser and Canvas in node and don't think that's possible, and making the node-canvas types assignable to the browser types is a ton of work without lying and causing possible runtime errors.

We also risk re-introducing #1656 if we reference any DOM types at all in the node path. So we might have to delete the browser entry point in the next major version...

@LinusU OffscreenCanvas would be cool. Do those Image/ImageData objects have enough features to work inside node-canvas? I'm nearly done with N-API and going to rewrite the font stack for Pango 2 as well, maybe we should start collecting what we want in v3 in a new issue.

[keean]

@chearon Another problem this is causing, is I used to be able to pass a CanvasRenderingContext2D from node-canvas to 'pdfjs' to render a PDF to it in Node (using TypeScript). This no longer works because pdfjs expects a browser 'CanvasRenderingContext2D'... and it appears the NodeCanvasRenderingContext2D is no longer compatible (cannot be passed as a CanvasRenderingContext2D in TypeScript (and would probably result in runtime errors in JavaScript?).

I expect this will prevent many JS/TS libraries from being usable server-side, that used to work with node-canvas.

[chearon]

Right. It would be an error to pass a node-canvas context where a browser context is expected since they are not the same. pdfjs could duck-type its argument to a subset of draw commands but we probably can't expect library authors to do that. You'll probably have to use as CanvasRenderingContext2D until we can find a better solution.

[keean]

@chearon That cast 'as' is not allowed because CanvasRenderingContext2D is not a subtype of NodeCanvasRendering2D. You have to do a two step cast like this:

context as unknown as CanvasRenderingContext2D

Which is of course totally unsafe, context could be anything, and runtime errors become possible (but it does work for now).

A base class for node-canvas that is the same as the browser CanvasRenderingContext2D, and then an extension for all the extra stuff in NodeCanvasRenderingContext2D would allow safe downcasting at least.

[zbjornson]

A base class for node-canvas that is the same as the browser CanvasRenderingContext2D

I don't think this is possible while also providing accurate types. node-canvas lacks some standard things like canvas.addEventListener(), all methods and properties inherited from Element, ctx.direction, ctx.createConicGradient(), ctx.scrollPathIntoView(), ctx.isPointInStroke(), etc. I haven't updated https://github.com/Automattic/node-canvas/wiki/Compatibility-Status in a while, but there's a lot of standard stuff node-canvas doesn't and can't support.

[LinusU]

@LinusU OffscreenCanvas would be cool. Do those Image/ImageData objects have enough features to work inside node-canvas?

I think so, as far as I remember they support everything that their browser equivalents does, and there is also support for getting the actual data out of them with an easy function.

I'm nearly done with N-API and going to rewrite the font stack for Pango 2 as well, [...]

Amazing 🤩 🤩

[...] maybe we should start collecting what we want in v3 in a new issue.

Let's do it! I've opened an issue 👉 #2232