MAJOR feat(fabric) remove callbacks in for Promise support #7657

asturur · 2022-02-06T16:24:49Z

Scope of changes:
fabric.Image functionalities
loadFromJSON, fromObject functionalities.

SVG hasn't be touched yet.

BREAKING:
Everything that was

function(arg, callback, arg2, arg3)

is now

function(arg, arg2, arg3).then(callback)

All places i found that take a string, enlive the content of the string and set the properties and accept a callback, have been removed, including:

Canvas.setBackgroundImage
Canvas.setOverlayImage
new Pattern({ source: string })

The suggest replacements are:

fabric.Image.fromUrl(url).then((fabricImage => canvas.backgroundImage = fabricImage));
or use Pattern.fromObject for the pattern.

Those seems reasonbale changes for the sake of standardization.

src/shapes/group.class.js

src/shapes/image.class.js

src/shapes/itext.class.js

src/shapes/line.class.js

src/shapes/path.class.js

src/shapes/text.class.js

src/util/lang_object.js

src/util/misc.js

asturur · 2022-02-06T16:31:28Z

src/util/misc.js

-          onLoaded();
+    enlivenObjects: function(objects, namespace, reviver) {
+      return Promise.all(objects.map(function(obj) {
+        var klass = fabric.util.getKlass(obj.type, namespace);


if we find a nice solution for filters, namespace can go away

YES! Go away namespace (continuing previous comment)

And when it goes away I hope it takes it's friend fabric.util.getKlass

same here regarding passing klass instead of namespace

asturur · 2022-02-06T16:34:46Z

@ShaMan123 @melchiar When i know it actually runs i will ask you to give a look.
For now is wasted time

.eslintrc.json

ShaMan123 · 2022-02-06T17:49:43Z

Holy shit this is major!
So much garbage removed! Exciting!

on-behalf-of: @shutterstock <abogazzi@shutterstock.com>

on-behalf-of: @shutterstock <andreabogazzi79@gmail.com>

src/mixins/canvas_serialization.mixin.js

src/util/misc.js

asturur · 2022-02-12T07:21:26Z

There is a little bit of changes here.
Apart promises, things should work in this way:
When you create an object with var myThing = new fabric.MyThing(options), i wish the standard was that you pass in the options values that are ready to use, and not the serialized part of it.
Serialized things are async and i don't like async constructors or with callback. The general consensus is that constructor shouldn't do much work.

So enliveObjectEnlivables can eventually wrap the options before you create an object, or you start from the fromObject method directly.

Indeed i removed the setBackgroundImage, setOverlayImage.
if you have a source of an image:

fabric.Image.FromUrl(src).then((fabricImage) => canvas.backgroundImage = fabricImage)

From a developer persepctive seems reasonable to write something like that.

ShaMan123

This PR looks awesome.
Makes the codebase much more readable and elegant and easy to use and override.
I have added comments.
The main things are:

keeping customization in mind (e.g. introducing a param/mechanism to allow adding custom enlivened props with ease)
drop namespaces altogether
be consist in dropping callbacks - basically drop _fromObject as well

src/util/misc.js

src/mixins/canvas_serialization.mixin.js

src/shapes/image.class.js

src/shapes/object.class.js

src/util/lang_object.js

src/util/misc.js

ShaMan123 · 2022-02-12T09:50:32Z

src/util/misc.js

-          onLoaded();
+    enlivenObjects: function(objects, namespace, reviver) {
+      return Promise.all(objects.map(function(obj) {
+        var klass = fabric.util.getKlass(obj.type, namespace);


YES! Go away namespace (continuing previous comment)

ShaMan123 · 2022-02-12T09:51:08Z

src/util/misc.js

-          onLoaded();
+    enlivenObjects: function(objects, namespace, reviver) {
+      return Promise.all(objects.map(function(obj) {
+        var klass = fabric.util.getKlass(obj.type, namespace);


And when it goes away I hope it takes it's friend fabric.util.getKlass

src/shapes/line.class.js

asturur · 2022-02-12T20:21:30Z

be consist in dropping callbacks - basically drop _fromObject as well
What do you mean with dropping _fromObject?

ShaMan123 · 2022-02-12T20:50:18Z

be consist in dropping callbacks - basically drop _fromObject as well
What do you mean with dropping _fromObject?

I think it is redundant - it doesn't do much but wrap the call to fabric.util.enlivenObjectEnlivables
Perhaps rename it as something like prepareObject

asturur · 2022-02-13T06:54:19Z

be consist in dropping callbacks - basically drop _fromObject as well
What do you mean with dropping _fromObject?

I think it is redundant - it doesn't do much but wrap the call to fabric.util.enlivenObjectEnlivables Perhaps rename it as something like prepareObject

Well if it wraps if for 6-7 classes, is good enough.
When switching to es6 or ts, most of those things shrink in size considerably.

asturur · 2022-02-13T06:57:18Z

I don't see the reason to keep line (isn't polyline a better version of it?)

in theory both Line, Triangle, Rect, Polyline and Polygon can be just constructors that rely on the underlying fabric.Path.

This is a long standing idea that was around, but we need to verify if the necessary setter/getter and code to make it still fill like the previous object is convenient compared to keep them separated

asturur · 2022-02-13T07:08:03Z

What about lodash?
Can we consider dropping all these utils (like extend) in favor of lodash?? It's better practice, reduces what fabric is in charge of and let's face it, lodash is excellent and fast.

Our deep extend had a different meaning compared to the lodash one.
Ideally we wouldn't need one at all
If the stateful approach is removed, the only other use we do is to protect from mutations the object options when we instantiate classes, but between destructuring and other things we can probably get rid of our special object_lang file

github-actions · 2022-02-15T18:41:03Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.33 |    76.74 |   86.51 |   83.06 |                                               
 fabric.js |   83.33 |    76.74 |   86.51 |   83.06 | ...,29662,29787,29867-29932,30055,30154,30371 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-02-15T18:52:09Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.28 |    76.71 |   86.51 |      83 |                                               
 fabric.js |   83.28 |    76.71 |   86.51 |      83 | ...,29662,29787,29867-29932,30055,30154,30371 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-02-15T19:10:08Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.28 |    76.71 |   86.51 |      83 |                                               
 fabric.js |   83.28 |    76.71 |   86.51 |      83 | ...,29663,29788,29868-29933,30056,30155,30372 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-02-15T19:30:21Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.28 |    76.71 |   86.51 |      83 |                                               
 fabric.js |   83.28 |    76.71 |   86.51 |      83 | ...,29661,29786,29866-29931,30054,30153,30370 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-02-15T19:41:38Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.28 |    76.71 |   86.51 |      83 |                                               
 fabric.js |   83.28 |    76.71 |   86.51 |      83 | ...,29659,29784,29864-29929,30052,30151,30368 
-----------|---------|----------|---------|---------|-----------------------------------------------

ShaMan123

Eraser is committed
I've pushed some minor JSDOC changes as well

I want to change enlivenObjects not to use getKlass at all (we'll do that once we migrate js)
I do think we should consider wrapping the external methods of the svg parser with promises (and leave internals as is until you decide it's fate) in order to unify fabric. But it can wait.

Excellent job! This was a huge tedious effort.
Group is up!

melchiar

This is great. So much garbage removed!

src/mixins/canvas_serialization.mixin.js

asturur · 2022-02-16T08:18:28Z

@ShaMan123 we will solves svg before getting to next version, either wrapping or changing the parser.

github-actions · 2022-02-16T08:25:07Z

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.28 |    76.71 |   86.51 |      83 |                                               
 fabric.js |   83.28 |    76.71 |   86.51 |      83 | ...,29659,29784,29864-29929,30052,30151,30368 
-----------|---------|----------|---------|---------|-----------------------------------------------

asturur · 2022-02-16T09:06:32Z

off i pushed away reviews because i addressed the text comment.

rhanmiano · 2022-03-26T17:27:39Z

Hello, is this feature available already, and in which version? Thanks

ShaMan123 · 2022-03-26T17:29:03Z

v5+

rhanmiano · 2022-03-27T04:01:08Z

Hi @ShaMan123 , thanks for the response. I just have a problem. After upgrading from 4.6.0 to 5.2.1. I get an error similar to this using loadFromJSON(...).then. After inspecting the dist/fabric.js the structure of loadFromJSON is still using the fn(json, callback, reviver). Am I missing something? Thank you.

Please see image for reference.

ShaMan123 · 2022-03-27T04:21:23Z

Did you try building fabric as suggested in the issue you referenced?

rhanmiano · 2022-03-27T04:27:55Z

I tried rebuilding the fabric also, and the generated fabric.min.js still contains the structure of fn(json, callback, reviver) just the minified version. And still having the same issue.

ShaMan123 · 2022-03-27T05:05:00Z

I see the version has somehow got mixed with outdated code.
I suggest you install fabric's latest version directly from github.

npm i --save github:fabricjs/fabric.js

@asturur URGENT

rhanmiano · 2022-03-27T10:44:22Z

Thanks @ShaMan123 , that solves the issue on my side. Will wait for the next stable patch.

this is all broken

498a72b