Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

canvas.Image lacks onload #2264

Open
Pomax opened this issue Jul 2, 2023 · 6 comments
Open

canvas.Image lacks onload #2264

Pomax opened this issue Jul 2, 2023 · 6 comments

Comments

@Pomax
Copy link
Contributor

Pomax commented Jul 2, 2023

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?.()
}
@zbjornson
Copy link
Collaborator

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)

@Pomax
Copy link
Contributor Author

Pomax commented Jul 3, 2023

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.

@LinusU
Copy link
Collaborator

LinusU commented Jul 24, 2023

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

@Pomax
Copy link
Contributor Author

Pomax commented Jul 24, 2023

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.

@LinusU
Copy link
Collaborator

LinusU commented Jul 24, 2023

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?

@Pomax
Copy link
Contributor Author

Pomax commented Jul 24, 2023

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")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants