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 node not working #22

Closed
obsidianart opened this issue Nov 13, 2017 · 13 comments
Closed

Canvas node not working #22

obsidianart opened this issue Nov 13, 2017 · 13 comments

Comments

@obsidianart
Copy link

I have a code which it seems the same as your example page

const mergeImages = require('merge-images');
const Canvas = require('canvas');

const sendMerged = function(email) {
    mergeImages(['../public/logo.png', '../public/710fb.png'], {
      Canvas: Canvas
    })
    .then //....

But I get the error

TypeError: options.Canvas is not a constructor
    at /Users/xxx/GIT/projects/abc/node_modules/merge-images/dist/index.umd.js:25:32

If I console log Canvas I have the following

{ Canvas: { [Function: Canvas] _registerFont: [Function: _registerFont] },
  Context2d: [Function: CanvasRenderingContext2D],
  CanvasRenderingContext2D: [Function: CanvasRenderingContext2D],
  CanvasGradient: [Function: CanvasGradient],
  CanvasPattern: [Function: CanvasPattern],
  Image: { [Function: Image] MODE_IMAGE: 1, MODE_MIME: 2 },
  ImageData: [Function: ImageData],
  //...
}

So I tried to change the code above to

//...
const Canvas = require('canvas').Canvas; //This is against the canvas documentation example
//...

After this change it seems to be fine with the constructor but it fails the line after with

TypeError: Image is not a constructor
    at /Users/xxx/GIT/projects/abc/node_modules/merge-images/dist/index.umd.js:39:13

I wonder if it's a bug with the new version of a dependency (I noticed you don't pin them).
Do your test run a new npm install? is this functionality tested?
Thanks a lot

@lukechilds
Copy link
Owner

lukechilds commented Nov 14, 2017

Thanks for reporting, there are no dependencies in package.json, just dev dependencies. canvas is an optional external dependency.

But yeah, it looks to be a breaking change in the latest version of canvas. Testing now.

@lukechilds
Copy link
Owner

Ok, so this only breaks in canvas v2 which is still in alpha. Latest v1 works fine.

But running npm install canvas currently gives you canvas@2.0.0-alpha.6 which is odd. I have a fix working locally but kind of reluctant to merge it in until canvas v2 is stable.

@obsidianart
Copy link
Author

I would agree to avoid an unstable release, but probably it would be a good idea to add instruction about how to install the correct version of Canvas (or add optional code to work on both).
Thanks a lot for your reply.
(I checked the PR and it actually feel safe to work on both)

@lukechilds
Copy link
Owner

Well normally I'd say if someone is using an alpha package, the onus is on them to get it working. However this situation is slightly different, because installing canvas from npm defaults to the latest v2 alpha. A lot of users probably won't even realise they're using an alpha release.

I don't have an issue merging this in if the API is unlikely to change. I will raise an issue on the node-canvas repo.

@obsidianart
Copy link
Author

I totally agree with you, but many libraries I've see over time do similar strange things and use alpha as stable (ghost is a good example of the never ending alpha-beta game).
They also incorrectly state

Alpha versions of 2.0 can be installed using npm install canvas@next.

So my guess is that they pushed by mistake a release on the main repo

@lukechilds
Copy link
Owner

lukechilds commented Nov 14, 2017

Yeah, I noticed that too, opened an issue for clarification.

@LinusU
Copy link

LinusU commented Nov 14, 2017

So my guess is that they pushed by mistake a release on the main repo

Exactly what happened, fixed now 👌

@obsidianart
Copy link
Author

amazing \o/
Thanks a lot both for your help

@lukechilds
Copy link
Owner

Great, as soon as canvas@2 is released I'll merge #23

@LinusU
Copy link

LinusU commented Nov 15, 2017

Sounds smart, we cannot guarantee that that api will be the final 2.x 👍

@chernikovalexey
Copy link

chernikovalexey commented Dec 27, 2018

Hey, it's 2.2.0 (I'm talking about canvas) now but the problem actually still persists even on stable releases. I've had to install a 1-year-old 1.6.7 version to make marge-images working with canvas... With newer versions the script bails out with a "no constructor" error.

@TobiTenno
Copy link

here's a snippet that makes it work for me, @chernikovalexey:

const { Canvas, Image } = require('canvas');
Canvas.Image = Image;

then you can pass Canvas as normal to the options. I'm going to make a PR to update the docs to specify how this can be done.

@lukechilds
Copy link
Owner

Thanks for your work @TobiTenno but I think merge-images should just be updated to work with canvas@2 rather than patch canvas@2 to expose a canvas@1 compatible API.

canvas@2 has been out for a while now so support for canvas@1 can be dropped.

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

Successfully merging a pull request may close this issue.

5 participants