Use mock canvas object replacing `canvas` #3518

wimrijnders · 2017-10-03T07:27:02Z

A mock canvas object is added to the unit tests, which makes usage of module canvas voluntary.

The issue with canvas is that it requires an external dependency to cairo. This complicates setting up a develop environment for vis.js

Removed canvas from package.json
Added section to README.md with instructions on how to install canvas instead.

Fixes almende#3515. A mock canvas object is added to the unit tests, which makes usage of module `canvas` voluntary. The issue with `canvas` is that it requires an external dependency to `cairo`. This complicates setting up a develop environment for `vis.js` - Removed `canvas` from `package.json` - Added section to README.md with instructions on how to install `canvas` instead.

mojoaxel · 2017-10-04T08:05:12Z

package.json

@@ -33,30 +33,30 @@
    "watch-dev": "gulp watch --bundle"
  },
  "dependencies": {
+    "canvas": "^1.6.7",


"Canvas" is not needed als dependency because it is only used during development. Should be removed here.

Right. I must have placed it there again during testing.

mojoaxel · 2017-10-04T08:06:11Z

README.md

@@ -322,6 +322,31 @@ module: {

 There is also an [demo-project](https://github.com/mojoaxel/vis-webpack-demo) showing the integration of vis.js using webpack.

+
+## Using module `canvas`


I think this section should be better a sub-part of "Installation".

mojoaxel · 2017-10-04T08:06:52Z

README.md

+The unit tests use a mock object for canvas, which is severely limited in its functionality. It might be preferable
+to use module `canvas` supplies a canvas object for usage with `node.js`. This has a functional implementation of the 2D context.
+
+Reasons to use `canvas:


I don't think we need this "Reasons to use canvas" section at all.

mojoaxel · 2017-10-04T08:07:16Z

README.md

+The unit tests will only use a mock object for canvas, if there is no `canvas` element already supplied. So, if the `canvas` module is installed, that will take precedence.
+
+
+The issue with `canvas` is that it has an external dependency to `cairo`. This needs to be installed outside of the regular install as done by `npm`.


mojoaxel · 2017-10-04T08:08:41Z

README.md

+
+The issue with `canvas` is that it has an external dependency to `cairo`. This needs to be installed outside of the regular install as done by `npm`.
+
+  - In order to fully install `canvas` on Windows, follow [these instructions](https://github.com/Automattic/node-canvas/wiki/Installation---Windows).


Why not just link to the canvas-wiki and remove this whole section.

mbroad · 2017-10-04T04:00:38Z

test/Network.test.js

 var {allOptions, configureOptions} = require('./../lib/network/options.js');
 //var {printStyle} = require('./../lib/shared/Validator');


+
+


Extra blank lines.

Typically, I use 2 blank lines to separate top level class/function declarations, single blank line to separate import groups. Two blank lines to separate imports from constants. Single blank line between nested functions.

I have my own guidelines which broadly match yours. But this was indeed a good observation.

mbroad · 2017-10-04T04:01:27Z

test/Network.test.js

@@ -353,6 +360,9 @@ describe('Network', function () {


 describe('Node', function () {
+  before(function() {
+//    if (!this.canvasPresent) this.skip();
+  });


These before functions should all be removed, imo

Yes, they should. Artifacts of a previous attempt at a fix.

mbroad · 2017-10-04T04:03:08Z

test/canvas-mock.js

+ * Adapted from: https://github.com/Cristy94/canvas-mock
+ */
+
+var myHackedInCanvas;    // Use one global canvas instance for all calls to createElement('canvas');


Why global?
If that's a good reason, can we call this globalCanvasMock?

OK, the comment might be a bit off. It's strictly speaking not a global because it doesn't reside in the global namespace. It's a static within the module. I'll adjust the comment.

And the name is silly; changing it to canvasMock.

mbroad · 2017-10-04T11:36:39Z

test/canvas-mock.js

+      // Following added for vis.js unit tests
+      //
+
+      measureText: function(text) {


Spacing looks off

FYI, this code is a module which I took from here and adapted it to own needs when I realized it wasn't completely fit for the purpose.

If think you mean this spacing?

measureText: function(text) { return { width:>> <<12*text.length, height: 14 }; }, };

Otherwise, I don't know what you mean. I have a habit of aligning related stuff, but indeed it's not needed here with only two lines.

Ah wait, there's tabs there. That's probably what you mean. Ah, right, that's from the original. I'll change it to spaces.

mbroad

Looks good

wimrijnders · 2017-10-05T03:34:16Z

@mbroad Thanks. It's admittedly a hack, but it only runs within the unit tests so I think we're OK.

yotamberk · 2017-10-05T12:10:09Z

@mojoaxel can you please review again/accept changes so we can get this in?

mbroad · 2017-10-08T04:07:32Z

@wimrijnders - Seems like an appropriate use of a mock to me.

wimrijnders · 2017-10-13T07:54:42Z

@mojoaxel I've handled your review points. Can you please have a look again?

mojoaxel · 2017-10-13T20:45:31Z

We should release this quickly as a Patch-Release.

yotamberk · 2017-10-13T20:49:37Z

@mojoaxel Another release will occur in exactly 2 weeks

* Use mock canvas object replacing `canvas` Fixes almende#3515. A mock canvas object is added to the unit tests, which makes usage of module `canvas` voluntary. The issue with `canvas` is that it requires an external dependency to `cairo`. This complicates setting up a develop environment for `vis.js` - Removed `canvas` from `package.json` - Added section to README.md with instructions on how to install `canvas` instead. * Removed debugger statements * Updates for review * Fixes for review

wimrijnders added the source/non-public API label Oct 3, 2017

wimrijnders requested a review from mojoaxel October 3, 2017 07:27

Removed debugger statements

8f0cd56

mojoaxel suggested changes Oct 4, 2017

View reviewed changes

Updates for review

01fd176

mbroad reviewed Oct 4, 2017

View reviewed changes

Fixes for review

38de83a

mbroad approved these changes Oct 5, 2017

View reviewed changes

mojoaxel approved these changes Oct 13, 2017

View reviewed changes

mojoaxel added this to the Patch Release v4.21.1 milestone Oct 13, 2017

yotamberk merged commit 06b2623 into almende:develop Oct 13, 2017

wimrijnders deleted the mockCanvas branch October 14, 2017 02:50

wimrijnders mentioned this pull request Oct 15, 2017

Week scale - enhance and add to auto-scaling #3549

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use mock canvas object replacing `canvas` #3518

Use mock canvas object replacing `canvas` #3518

wimrijnders commented Oct 3, 2017

mojoaxel Oct 4, 2017

wimrijnders Oct 4, 2017

mojoaxel Oct 4, 2017

mojoaxel Oct 4, 2017

mojoaxel Oct 4, 2017

mojoaxel Oct 4, 2017

mbroad Oct 4, 2017

wimrijnders Oct 4, 2017

mbroad Oct 4, 2017

wimrijnders Oct 4, 2017

mbroad Oct 4, 2017

wimrijnders Oct 4, 2017

wimrijnders Oct 4, 2017

mbroad Oct 4, 2017

wimrijnders Oct 4, 2017

mbroad left a comment

wimrijnders commented Oct 5, 2017

yotamberk commented Oct 5, 2017

mbroad commented Oct 8, 2017

wimrijnders commented Oct 13, 2017

mojoaxel commented Oct 13, 2017

yotamberk commented Oct 13, 2017

		@@ -322,6 +322,31 @@ module: {

		There is also an [demo-project](https://github.com/mojoaxel/vis-webpack-demo) showing the integration of vis.js using webpack.


		## Using module `canvas`

		The unit tests will only use a mock object for canvas, if there is no `canvas` element already supplied. So, if the `canvas` module is installed, that will take precedence.


		The issue with `canvas` is that it has an external dependency to `cairo`. This needs to be installed outside of the regular install as done by `npm`.


		The issue with `canvas` is that it has an external dependency to `cairo`. This needs to be installed outside of the regular install as done by `npm`.

		- In order to fully install `canvas` on Windows, follow [these instructions](https://github.com/Automattic/node-canvas/wiki/Installation---Windows).

		var {allOptions, configureOptions} = require('./../lib/network/options.js');
		//var {printStyle} = require('./../lib/shared/Validator');

Use mock canvas object replacing canvas #3518

Use mock canvas object replacing canvas #3518

Conversation

wimrijnders commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbroad left a comment

Choose a reason for hiding this comment

wimrijnders commented Oct 5, 2017

yotamberk commented Oct 5, 2017

mbroad commented Oct 8, 2017

wimrijnders commented Oct 13, 2017

mojoaxel commented Oct 13, 2017

yotamberk commented Oct 13, 2017

Use mock canvas object replacing `canvas` #3518

Use mock canvas object replacing `canvas` #3518