Node Canvas 2, fabric major bump, trimming chances

[asturur]

The problem to solve

Node canvas 2 is getting out, will require latest jsdom, that requires node 6.
Node canvas 2 improvements are nice and i would like to upgrade, we need to remove node 4 support and so we should bump fabric to 3.x

This thread is to ask what you think of those changes and if there is a part of the api you think is bad and that ypu would like to fix at the next breaking occasion.

Current situation

We could bump to 3 with the only change being node 4 and move on. Or we could introduce some other breaking changes.

Proposal

Things i would like to kill:
setBackgroundImage api, setGradient api.
I would prefer people to learn to create a gradient, and assign to the right property, no need to have a special api for that.

Adding promise support (without embedding a promise polyfill ).
Leaving callback support.in
If a callback is not provided, the method would return a promise.( maybe this can be done anyway no major bump required )

Breaking changes

node 4 removal
removing a couple of methods from canvas
removal of setPatternFill and setGradientFill from objects

[awehring]

I support the idea to implement some breaking changes now.

I guess, many devs are now updating theire applications from version 1 to version 2 or will do so soon. The ones that just did it, will still have a good overview of theire application.
So I think it is easier to incorporate some additional breaking changes now, than in a year or so.

Amd I would definitely welcome promise support.

Could you elaborate which "couple of methods from canvas" you are considering for removal?

[asturur]

setBackgroundInage and setBackgroundColor or whatever they are called. people could just create their background of choice ( group image gradient ) and set it on backgroundImage or backgroundColor, there is no need for an extra method that can take an Url, since we have other methods to create images from url, and those are extra and create confusion. Few people knows that any fabricObject can be used on backroundImage and any filler ( color, pattern, gradient ) can be used as backgroundColor

[awehring]

Interesting. Didn't know this either. The docs still say
backgroundImage Background image of canvas instance. Should be set via fabric.StaticCanvas#setBackgroundImage".

However, is most likely a small breaking change for anybody.

[AlexSergey]

I am misunderstanding... I use setBackgroundColor with Pattern for make transparent like background in our project - Cleverbrush. This background won't be able to export, this background excluded from layer list. This is only pattern in background. How can I do that functional without "setBackgroundColor"?

"Adding promise support" - sounds good!

Node canvas still is alpha. It is safe?
Which will be able a new features from migration to node-canvas 2?
How about it's performance?

[asturur]

so without setBackgroundColor, you would create a pattern and then do canvas.backgroundColor = pattern the method is really unnecessary.

NodeCanvas is still in alpha, but they are speeding up, and this is not a change we do in a week or two, but also gather developers opinions takes time, so i started now.

Whatever are the performances of nodeCanvas, that is the future. The new features will be developed just there.
Text works better for example.

[asturur]

@aggrosoft here

[kingschnulli]

So here is my list of things I'm doing via custom code now - maybe there is something which could be part of 3.0 (I don't know if anything of this is breaking):

• Remove the now obsolete and undocumented fabric.canvasModule option

• Disable touch scrolling when an element is selected, this is annoying if on mobile.

• Allow to define how textstyles work, currently there is no way to force by line formatting. Also the code is making me crazy for all of the text styling - maybe we can clean that up a little.

• Objects do not have real UUID, this is a pain when working with vuejs or react (state mapping). But is easy to do in userland.

• Currently there is no way of telling the canvas to use anything else then ActiveSelection class, means you can not have a custom ActiveSelection class without also overriding _createGroup and _groupSelectedObjects in the main Canvas class. This felt somehow unclean when copying the whole methods just to use another class. Maybe this could be improved?

• Touch interactivity is ugly currently, I replaced all of it with a custom hammerjs based implementation. But maybe this is just my feeling, I did not like that you have to hit a selected object directly to scale/rotate it. I can make a jsfiddle to give you a feeling for what I changed.

• Constraining object position and size is still no core feature - I really think this should be part of the core.

I would be willing to share my implementation for any of these things, I know this is a lot that could just be done in 2.x

Also +1 for finally using promises, currently I'm using wrapper functions for everything to use promises.

[asturur]

apart the first item all the rest can be added later or now, is not breaking.
If you have strong feeling on touch controls, that have been mostly broken for all the time, please feel free to propose something.
I have got a touch device, a surface, just to start fixing it, but i m lazy about it.

[asturur]

For any of those topics also you can open a discussion thread here on this repo. Is important to keep it clean about the how and why, explaining patterns on why implementing it in the userland is not good and what disadvantages brings.

[asturur]

cleaning up text style methods is probably breaking. I remember a huge pain working on text and trying to make it more normal. I probably failed. Please share your experience on the weird methods.

[kingschnulli]

One major annoying thing in the text methods is that findLineBoundaryLeft, findLineBoundaryRight, findWordBoundary etc. depend on _splitText() being called before hand. This is a side effect, I don't know if this is desirable. I just noticed this because I used these methods when overriding _set for the fill property.

Also, why are all of these methods private? There is no reason to lock them in, I would also propose a findLineStartIndex and findLineEndIndex method for usage in userland.

It was a real pain to wrap my head around all of the code - especially selectionStart/selectionEnd properties being set directly, this could be totally rewritten.

[asturur]

i guess you are talking about textbox? what index are talking about? the one of the char in the text string, the index in the grapheme array or the style index of a wrapped line in a textbox?

Text is so much complicated.

IMHO the first rewrite would be eliminating textbox and putting a wrap attribute to the normal text. Adapt the code on just 2 classes ( text and itext ) and then clean it up.

But is so hard and without interaction automated tests you break things all along the way.

[kingschnulli]

Yeah I know, I wrote a custom implementation for rendering Text in fabric 1.x and it was a nightmare. I was talking about I-Text, I don't event know why Textbox has to exists - you are right about doing this just with Text/IText. We should create another issue just for this, also there is no real reason to hold back 3.x for this - or what do you think?

[asturur]

this is not something that can be done quickly, i do not think it gets out with node canvas update.

[kingschnulli]

So jsdom 13 is out, what do we need to prepare for the new major release? Custom fonts are terrible with canvas 1.x and this fixes a lot in node rendering

[asturur]

we should start a branch, called fabric-3.x and test it.
Do not expect a new world because is node-canvas 2.0
SVG support is very good, and working on the latest code is good too.
Most of the text improvements can be obtained in the 1.x branch too forcing libpango in some way.

I will start the branch, feel free to open PRs against it in which you switch jsdom dep to 13. and we can start to test if UT passes.

[asturur]

https://github.com/fabricjs/fabric.js/tree/fabric-update-3.x <--- if you want to update jsdom to ^13.0.0 and node canvas 2 to ^2.0.0 we can start

need to remove node4 and node6 too ( kinda sad for node 6 tho )

[asturur]

Step 1 for example, fabricjs/fabric.js#5356, figure out how JSDOM 13 compared to 9, gets imported and initialized

[asturur]

i have no idea why i closed it. i think i was sleeping.

[asturur]

help here is needed, this may be interesting and help all the node-js users, i have some issue on everyday life and cannot really get on speed with this.
So if you want to collaborate please feel free to open PRs on top of the already open PR that is making the UPDATE and that fails to start completely

[kingschnulli]

I don't know if this is the right place to ask, but have you ever thought about migrating the whole build process to something more modern like webpack? Also how do you think about using es5/es6 constructs to allow for tree shaking etc. when using fabric in a project? Using Babel this could be easily transpiled to be used with the target browser stack

[asturur]

Yes i thought about it a lot.
Most of the fabricJS users wants to smash a js file in a script tag.
The more advanced devs ( a minority ) wants the es6 import/export tree shaking

So while this will be done sometime, this requires rewriting everything and is on the trail of some more nicer feature i would like to work on.

my personal priorities are:
canvas2 migration, customized controls.

Done those i would also work on that, would solve the problem of the large builds for everyone.
And would still keep possible produce static builds with custom features.

[asturur]

The point is also this is the only contribution i would not want someone to contribute, because if do not know the codebase like my hands, it takes me triple time to solve/debug/suggest solutions.

[ludbek]

How far are we from getting node-canvas 2 support?

[asturur]

need to fix last 2 issues labeled as 'fix-next' and i can release. If you do not mind the actual 2 known bugs you can start using master as 3.0

[ludbek]

Thanks for the awesome work.

[niels-van-den-broeck]

I know this one is closed but please keep in mind that this absolutely broke testing fabricjs with jest since they use an older jsdom version to keep support for node 6.

Might not be bad to add that into breaking

[asturur]

fabricjs now uses node-canvas 2 and jsdom 15 i think.
Is there any way to force jest to use newer jsdom?

[niels-van-den-broeck]

@asturur Sorry for not getting back to this. The solution we have found is using the jest-environment-jsdom-fifteen package. This does require us to upgrade to the proper version whenever fabricjs decides to do a jsdom upgrade.