[Bug]: Can't instantiate FabricImage with node-canvas Image since v6

[SvenFackert]

CheckList

• I agree to follow this project's Code of Conduct
• I have read and followed the Contributing Guide
• I have read and followed the Issue Tracker Guide
• I have searched and referenced existing issues and discussions
• I am filing a BUG report.
• I have managed to reproduce the bug after upgrading to the latest version
• I have created an accurate and minimal reproduction

Version

6.0.2

In What environments are you experiencing the problem?

Node.js

Node Version (if applicable)

20.0.0

Link To Reproduction

Description should be sufficient (references to specific code given)

Steps To Reproduce

1. Create an Image using node-canvas
2. Pass it to the FabricImage constructor

import { Image } from 'canvas';

const nodeCanvasImage = new Image();
const fabricImage = new FabricImage(nodeCanvasImage);

Expected Behavior

Similar to this bug report I would expect, that passing a node-canvas Image object still works. We were using fabric.js together with node-canvas for our node backends as previously described in the docs here. It would be nice to be able to use the node-canvas Image without stubbing/monkey patching the classList property as before. And also without adding js-dom just for this use case. What do you think?

Actual Behavior

The constructor of FabricImage will call Image.setElement(), which tries to call the classList.add() method on the passed image. The classList is only supported by HTMLImageElement and not node-canvas Image. This will result in the following runtime error: Cannot read properties of undefined (reading 'add').

Error Message & Stack Trace

No response

[asturur]

We keep JSDOM for this purpose.
are you importing fabricImage from 'fabric/node' ? or from 'fabric' ?

[asturur]

It wouldn't be an issue to avoid classList, but there may be other interactions we do with the image html element or that we may add in the future that would break again. Why you don't want to install JSDOM?

I'm also not sure why we add that class anymore.
Maybe we can make this check optional if it solves your issue, but that wouldn't make node-canvas images supported entirely.

[zhangweiwei0326]

+1 node_modules\fabric\dist\index.node.cjs:24117
element.classList.add(FabricImage.CSS_CANVAS);
^
TypeError: Cannot read properties of undefined (reading 'add')

[bkeepers]

Workaround for now is to add a classList property with an add merthod:

  import { Image } from 'canvas';

  const img = new Image();
+ img.classList = new Set();
  const fabricImage = new FabricImage(img);

[asturur]

I don't even know why we add that class. Like if you want to target some css on that element, but why?
Seems dangerous especially if you are reusing an element that is also on the dom and you are referencing it adding a class with attached style ( not provided by default ) is going to mess up with how that element look like.

I ll flag that for removal, i really can't understand the usefulness of it

[asturur]

I deprecated it.

[SvenFackert]

Hi everyone, thanks for tackling this issue! I appreciate it 🙏. Please let me know, when this change is published, so we can update fabric.js in our projects 👍

[SvenFackert]

We keep JSDOM for this purpose. are you importing fabricImage from 'fabric/node' ? or from 'fabric' ?

From 'fabric/node'

[asturur]

It will take time before is published because in theory is a breaking change.
What we can do meantime is add an optional chainer to make it work

[asturur]

Do you feel like you can do it and open a PR? is just a ? between the call and the parenthesis

[SvenFackert]

@asturur yes, sure. Here it is: #10412 👍 It's not urgent because I already monkey patched that in our project. But it might help others till the classList access gets removed in the future 👍

[asturur]

Yes but monkeypatching for those small things is evil.
If the fix makes sense better to have it.
Most of my effort are toward monkepatching be avoidable with apis and subclassing