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

Promises #2556

Closed
wants to merge 7 commits into from
Closed

Promises #2556

wants to merge 7 commits into from

Conversation

kangax
Copy link
Member

@kangax kangax commented Oct 15, 2015

  • Port all fromObject to promises (no more different behavior for sync/async objects)
  • Port fabric.util.loadImage
  • Port fabric.Image.fromURL
  • Port fabric.loadSVGFromURL
  • Port fabric.loadSVGFromString
  • Port objects' clone to make it consisten (similar to fromObject)
  • Port other methods that currently take callback? Which ones? All?
  • Port overwritten methods in node.js
  • Fix tests
  • Update demos
  • Update examples in docs
  • Update tutorials

@kangax
Copy link
Member Author

kangax commented Oct 15, 2015

@asturur @Kienz @inssein I'm wondering which other methods aside from fromObject (which needed normalization) should we port to promises? I started untangling methods one by one and realizing that perhaps it makes sense to remove all callbacks entirely! In other words, replace all async or potentially-async methods to return promises. What do you think? Any downsides I'm not seeing?

@asturur
Copy link
Member

asturur commented Oct 15, 2015

i need time to understand this changes and undesired effects.
i do not know promises.

@kangax
Copy link
Member Author

kangax commented Oct 15, 2015

@asturur I was confused about promises too, but they're actually very simple! It's a mechanism and API that allows us to transform callback-based methods to methods that can be chained instead.

Here's an example:

fabric.util.loadImage(url, function(img) { /* use img */ });

becomes:

fabric.util.loadImage(url)
  .then(function(img) { /* use img */ })
  .catch(function() { /* something went wrong */ })

As you can see, loadImage now returns something that has then and catch methods.

Now, another great thing is that you can chain promises, so you can do:

fabric.util.loadImage(url)
  .then(function(img) { /* do something with an img*/ return img })
  .then(function(img) { /* do more stuff */ })
  .then(/* ... */)

The advantages for us are:

  • Normalized API inside Fabric
  • Normalized API for errors (right now it's impossible to catch some of the fetch failures, like when loading SVG)
  • Standard API for when working with other libraries (i.e. you can pass return value of loadImage to another library and that library doesn't need to know if there's callback or not, which argument that callback is and so on; if it knows that it's promise, it knows that promise always has the same API)

@kangax
Copy link
Member Author

kangax commented Oct 15, 2015

This article explains it well — http://www.html5rocks.com/en/tutorials/es6/promises/
Also, if you used jQuery's $.ajax or $.get, it basically returns promise too. That's why you can do $.get(url).then(...) and so on.

@inssein
Copy link
Contributor

inssein commented Oct 15, 2015

@kangax awesome! seems like you are already down the path of replacing almost all callbacks, which is what I would have suggested. I think it cleans up the code and allows better error handling.

I will definitely be porting my application to use promises as well. Might be a little more painful for me as my extension of the image shape fires the image on load callback twice (once if a thumbnail is available and ready, and another one when the high res image is available). That said, I think using promises will clean up my implementation as well.

@kangax
Copy link
Member Author

kangax commented Oct 15, 2015

Wait a second.... are we even using _toDataURL and _toDataURLWithMultiplier from canvas_serialization.js anywhere? They look completely unused! /cc @asturur

and _toDataURLWithMultiplier (but are last 2 used anywhere?)
@inssein
Copy link
Contributor

inssein commented Nov 16, 2015

@kangax any timeline on this? Would be a nice thing to get merged, along with the jsdom changes so we can be node > 4.2 compat. Going to wait until at least those two are merged before I stop using my own fork of fabric that uses the newer jsdom, but doesn't have all the recent changes.

@kangax
Copy link
Member Author

kangax commented Nov 24, 2015

@inssein I just started a new job and have tons of stuff to do (again) so not sure when I'll have a chance to get to this! Sorry; will do my best but really no idea at the moment.

@asturur
Copy link
Member

asturur commented Aug 20, 2016

I think that now that i got acquatained with promises ( finally.. ) i ll release 1.6.4 and start some beta branch with 2.0 Promises and some more changes.
Filters need Promises to. They are mostly sync but still you could build some filter based on some external async lib and promise could come in handy.

@asturur asturur mentioned this pull request Feb 11, 2017
if (hasUrl) {
svgCache.get(url, function (value) {
var enlivedRecord = _enlivenCachedObject(value);
resolve(enlivedRecord.objects, enlivedRecord.options);

Choose a reason for hiding this comment

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

This is not valid; a Promise can only ever be resolved with a single value. With the current approach, the second value would simply be lost. To resolve with multiple values, resolve with an object (with named keys) or an array.

@nasiruddin-saiyed
Copy link

Any update regards this PR?? i am struggling to get it clean when working with more than 20 canvas in a single react component.

@asturur
Copy link
Member

asturur commented Dec 18, 2019

This PR should be rebased on latest version.

@stale
Copy link

stale bot commented Jan 25, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issue marked as stale by the stale bot label Jan 25, 2020
@asturur
Copy link
Member

asturur commented Jan 25, 2020

We should add promises, this PR cannot be updated after so long, i ll close it.

@asturur asturur closed this Jan 25, 2020
@asturur asturur deleted the feature/promises branch March 9, 2020 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issue marked as stale by the stale bot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants