Support canvas v2 #81
Conversation
tjenkinson
force-pushed
the
support-canvas-v2
branch
from
411e825
to
42c410b
Compare
| Canvas: Canvas, | ||
| Image: Image |
There was a problem hiding this comment.
since this limits the usability to only canvas v2 (since it's a dev, not full dependency, and the consumer would have to provide it, i think it'd be better to document how canvas v2 could be used on the consumer's end instead of forcing it through this route.
There was a problem hiding this comment.
I'm not quite following. merge-images would still work with canvas v1. In the v1 case the Image property would just be omitted.
There was a problem hiding this comment.
Ah, you're right, I was misreading this as the source, not the markdown. I was thinking something similar but just doing doc for it. Sorry for my misread
|
@tjenkinson thank you so much for this! 🙏 A very simple change but I just haven't had the chance to take a look and see what needed changing since v2 was released and #23 was broken. |
| canvas.toDataURL(options.format, { | ||
| quality: options.quality, | ||
| progressive: false | ||
| }, (err, jpeg) => { | ||
| if (err) { | ||
| throw err; | ||
| reject(err); |
There was a problem hiding this comment.
How come this is changed to reject instead of throw?
There was a problem hiding this comment.
the error was being thrown from an asynchronous function, meaning it would not result in the promise being rejected.
The function provided to Promise will reject automatically if it catches an exception, but it won't catch it from something asynchronous.
| @@ -9,5 +9,5 @@ test('mergeImages rejects Promise if node-canvas instance isn\'t passed in', asy | |||
|
|
|||
| test('mergeImages rejects Promise if image load errors', async t => { | |||
| t.plan(1); | |||
| await t.throws(mergeImages([1], { Canvas })); | |||
| await t.throws(mergeImages(['nothing-here.jpg'], { Canvas, Image })); | |||
There was a problem hiding this comment.
How come this is changed to 'nothing-here.jpg' instead of 1?
There was a problem hiding this comment.
Without this it now fails with
errors › mergeImages rejects Promise if image load errors
/Users/tomjenkinson/Documents/GitHub/merge-images/node_modules/nyc/node_modules/signal-exit/index.js:155
Promise returned by test never resolved
I investigated a bit further and the reason is that Image doesn't call onerror (or onload) when it's given a number. Chrome does call onerror, so this probably should be a bug report on Canvas
Co-authored-by: Luke Childs <[email protected]>
|
Thanks for this @tjenkinson! Published as Browser users are unaffected though. |
In canvas v2
Imageis not onCanvas.See https://github.com/Automattic/node-canvas/blob/master/CHANGELOG.md#200
This adds a
Imageto the options, and also throws a specific error ifImageshould probably be provided.