upgrade jsdom

[inssein]

Just upgraded jsdom to 6.5.x. Tested against node v4.2.0 and canvas 1.2.10. Not sure what this means for previous versions of node.

[inssein]

Changed the title to add a [WIP] as this may involve more work than I assumed. I have my own application working with Node v4.2.1, but its clear that some things I don't use are changed, and the tests indicate that.

One of the main problems is that jsdom doesn't return a node canvas element anymore, it returns a wrapper (HTMLCanvasElement). The node canvas object is accessible via _nodeCanvas on the canvas element (https://github.com/tmpvar/jsdom/blob/08bc013da96bde74607cf7d6b9ff51a56e7931aa/lib/jsdom/level2/html.js#L1204)..

Edit: From reading the changelog. the change mentioned above came in 6.3.0 in order to unify how jsdom works.

[inssein]

@kangax should I be pursuing this further? The path that I am currently headed down doesn't allow for a backwards compatible fabric.js due to the changes mentioned above.

There could be some potentially backwards compatible ways of moving forward, such as trying to use a version of jsdom prior to 6.3. (I haven't confirmed that jsdom 6.3 works on node > 4)

[kangax]

@inssein I'm not entirely sure (need to look into it more when I get a chance) but fwiw we've done some back-incompat changes and are technically on the verge of 2.0 (since we follow semver now). Perhaps we can use that opportunity to update JSDom.

[inssein]

@kangax sounds good. I just made node 4+ pass on travis. The fix was a lot easier than I thought.

Edit: Removed the [WIP] tag as everything now works, but only on node 4+ and is backwards incompatible.

[inssein]

@kangax is it okay if i rebuild the library and commit the build files in this PR? How else do most people make PR and use it as a part of a node app?

[kangax]

How else do most people make PR and use it as a part of a node app?

IIRC, you can refer to a commit in package.json

[CombatCode]

current master + your changes on node 4.2.1 , I will check 4.2.2 at the moment

[inssein]

@CombatCode I can try this again on Monday, but it seems like you missed a step perhaps? The error is pretty much saying "fabric.window" wasn't defined properly, which is not possible if jsdom is properly installed.

[CombatCode]

There is a README for jsdom in ver. 6.5.1.
Fabric.window is based on fabric.document.parentwindow but they are not using anymore parentwindow in favor of defaultView

[inssein]

If you look at my commit, that's exactly what I'm using.

On Tue, Oct 13, 2015, 3:53 PM Juriy Zaytsev [email protected]
wrote:

@inssein https://github.com/inssein I'm not entirely sure (need to look
into it more when I get a chance) but fwiw we've done some back-incompat
changes and are technically on the verge of 2.0 (since we follow semver
now). Perhaps we can use that opportunity to update JSDom.

—
Reply to this email directly or view it on GitHub
#2547 (comment).

[CombatCode]

ahh shit, that's true, I have lost a bit time ;) but after installing package via npm, the dist/ folder is in use with old code https://github.com/kangax/fabric.js/blob/master/dist/fabric.js#L23

Anyway, thanks, your commit was very helpful for me

[asturur]

this looks like a big change.
or am i wrong? looks like that canvasEl is no more the same between browser and nodecanvas

[inssein]

It's not a big change. Canvas was never the same between browser and node. The previous version of jsdom just returned the canvas when you did fabric.document.createElement('canvas'), but in the new version of jsdom, they create a wrapper around the canvas returned, but have it accessible via _nodeCanvas. (https://github.com/tmpvar/jsdom/blob/master/lib/jsdom/level2/html.js#L1472)

[asturur]

i was thinking about other places where we use the canvasEl directly. But i didn't found yet.
Other places we use just the context. Maybe in (StaticCanvas / Canvas).getElement() we should make the same check and return.

Also we use
in other places like:
https://github.com/kangax/fabric.js/blob/master/dist/fabric.js#L780
https://github.com/kangax/fabric.js/blob/master/dist/fabric.js#L6246
https://github.com/kangax/fabric.js/blob/master/dist/fabric.js#L8052

Some of them returns the element directly.

[asturur]

replaced by #2872