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

Update to latest createjs libs, latest canvas, working on recent node version #3

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

jedateach
Copy link

@jedateach jedateach commented Jul 30, 2019

In short this PR:

  • Updates the node canvas package to version 2
  • Includes easeljs via npm, which gets required by the main file, and also gets served in the examples server.
  • Has been coded on node v8.9

I chose to call my branch 1.0, because if you follow semantic versioning, then this is a breaking change to existing projects. Accordingly, I've bumped the package.json's version number to 1.0.0.

I've broken this work up into a few commits, but can squash it all down, if desired.

In doing this work, I realised there have actually been a few others work on the same issues. I've had a quick look through, and their changes do appear to be covered by this PR.

@@ -1,6 +1,5 @@
var fs = require('fs');
var Canvas = require('canvas');
var Image = Canvas.Image;
var { Canvas, Image } = require('canvas');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The canvas API changed in version 2.

Using this destructing syntax will break older versions of node. We can degrade it back to older syntax, if needed. But really a decision needs to be made on the minimum node version, and subsequently it should run on a CI test matrix that includes includes that minimum version of node.

/**
* Helper for reporting back exec errors
*/
function execCallback(error, stdout, stderr) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helped reveal a problem in ffmpeg output. The cli tool has probably changed a bit since this work was originally done.

@@ -13,13 +13,12 @@
"images",
"easel"
],
"homepage":"https://github.com/wdamien/node-easel",
"repository":"git://github.com/wdamien/node-easel",
Copy link
Author

@jedateach jedateach Jul 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These urls are published on the npm registry: https://www.npmjs.com/package/node-easel

...which made it confusing that the main repo is actually under the CreateJS github organisation.

@jedateach jedateach marked this pull request as ready for review July 30, 2019 00:22
@SvetlozarValchev
Copy link

Thanks for this, it saved me tons of time researching how to get it to latest version.

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 this pull request may close these issues.

2 participants