canvas.Image should call onload asynchronously when src is set

Issue or Feature

The Image implementation lacks an onload call (at least based on https://github.com/Automattic/node-canvas/blob/master/lib/image.js), making it impossible to use it "the normal way" where the onload is our signal that image data is available and now we can do something with that data.

(onload on the HTML side = incredibly bad, do not use. onload on the JS side: unfortunately still extremely necessary for Image, WebSocket, etc. etc. it never went away in JS land and we kept reinforcing it by introducing new object types that kept the onload and onerror pattern firmly alive right up to today =S)

Steps to Reproduce

const IN_BROWSER = !!globalThis.document;

let Image = globalThis.Image;
let createCanvas = (w, h) => {
  const cvs = globalThis.document.createElement(`canvas`);
  cvs.width = w;
  cvs.heigh = h;
 return cvs;
};

if (!IN_BROWSER) {
  const { createCanvas as create, Image as ImageClass } = await import(`canvas`);
  createCanvas = create;
  Image = ImageClass;
}

const img = new Image();
img.src = "./some/path.png";
img.onload = () => {
  // actually do things here, because we like idempotent code that just runs,
  // irrespective of whether it's in the browser or Node.js
  const myCanvas = createCanvas(...);
};

The above code won't actually do anything for node-canvas (but will run perfectly fine in the browser) because the shimmed Image has no code to trigger the onload "callback".

Suggested fix

Add an onload property with getter/setter:

Object.defineProperty(Image.prototype, 'onload', {
  get: function() { return this._onload },
  set: function(fn) {
    this._onload = fn;
    if (this.src) fn();
  }
);

And update setSource to something like

function setSource (img, src, origSrc) {
  SetSource.call(img, src)
  img._originalSource = origSrc
  img._onload?.()
}

It's defined in C++:

node-canvas/src/Image.cc

Lines 92 to 93 in adf73ee

    
           Nan::Set(info.This(), Nan::New("onload").ToLocalChecked(), Nan::Null()).Check(); 
        
           Nan::Set(info.This(), Nan::New("onerror").ToLocalChecked(), Nan::Null()).Check();

node-canvas's img.src= is sync right now though, so you need to set onload before src. (#1710)

That's not really in keeping with how it works in the browser though, so updating the JS side to make sure onload also gets called if it gets assigned after src has been assigned would be a good code update.

It's not ideal, but I don't think we can change anything in the current version without that being a breaking change.

Having img.src= be async (as the browsers do it) is already planned for Canvas v3: #2232

why would that be a breaking change? existing code will keep working the same way, and "the browsery way" currently doesn't work, so isn't being used. onload only gets to trigger once per src assignment, so any code folks already wrote should be entirely unaffected.

should being key here, e.g. this code snippet would have changed behaviour?

const img = new Image()

img.src = 'a.png'
img.onload = () => console.log('test')
img.src = 'b.png'

Before this logged test once, but if we implement your suggested change as I've understood it, it would log test twice?

That'd be odd code to have anywhere, but yes, that would break. Then again, that's why semver exists of course: just bump the major version so that npm and friends don't auto-uplift to a breaking change. There's nothing sacred about version numbers, they're just a versioning utility, the "it'd be a breaking change" argument is literally why semver exists. Unless #2232 is planned for soon, of course, but from the looks of it, it doesn't have a timeline and none of the tasks have been completed yet?

(Breaking changes should be fine, no matter how frequent, just bump the major version number each time you add one. Because the major version does not mean "a completely reworked release" when you're using semver. It just means "this is not backward compatible in one or more ways")

Navigation Menu

Repository navigation

canvas.Image should call onload asynchronously when src is set #2264

Issue or Feature

Steps to Reproduce

Suggested fix

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	Nan::Set(info.This(), Nan::New("onload").ToLocalChecked(), Nan::Null()).Check();
	Nan::Set(info.This(), Nan::New("onerror").ToLocalChecked(), Nan::Null()).Check();

canvas.Image should call onload asynchronously when src is set #2264

Description

Issue or Feature

Steps to Reproduce

Suggested fix

Activity

zbjornson commented on Jul 3, 2023

Pomax commented on Jul 3, 2023

LinusU commented on Jul 24, 2023

Pomax commented on Jul 24, 2023

LinusU commented on Jul 24, 2023

Pomax commented on Jul 24, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions