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 support #3684

Open
joepie91 opened this Issue Feb 11, 2017 · 11 comments

Comments

Projects
None yet
4 participants
@joepie91
Copy link

joepie91 commented Feb 11, 2017

Currently, something like fabric.Image.fromURL only supports callbacks. Usage example from the tutorial:

fabric.Image.fromURL('/assets/pug.jpg', function(img) {
  var img1 = img.scale(0.1).set({ left: 100, top: 100 });

  fabric.Image.fromURL('/assets/pug.jpg', function(img) {
    var img2 = img.scale(0.1).set({ left: 175, top: 175 });

    fabric.Image.fromURL('/assets/pug.jpg', function(img) {
      var img3 = img.scale(0.1).set({ left: 250, top: 250 });

      canvas.add(new fabric.Group([ img1, img2, img3], { left: 200, top: 200 }))
    });
  });
});

My request would be to add Promises support out of the box for fromURL, and any other asynchronous calls in the library, such that you could instead write the following:

Promise.all([
    fabric.Image.fromURL('/assets/pug.jpg'),
    fabric.Image.fromURL('/assets/pug.jpg'),
    fabric.Image.fromURL('/assets/pug.jpg')
]).then(([img1, img2, img3]) => {
    let transformedImg1 = img1.scale(0.1).set({ left: 100, top: 100 });
    let transformedImg2 = img2.scale(0.1).set({ left: 175, top: 175 });
    let transformedImg3 = img3.scale(0.1).set({ left: 250, top: 250 });

    canvas.add(new fabric.Group([ transformedImg1, transformedImg2, transformedImg3], { left: 200, top: 200 }))
});

... or even something like this, using Bluebird:

Promise.all([
    fabric.Image.fromURL('/assets/pug.jpg'),
    fabric.Image.fromURL('/assets/pug.jpg'),
    fabric.Image.fromURL('/assets/pug.jpg')
]).map((img, i) => {
    let offset = 25 + (i * 75);
    return img.scale(0.1).set({ left: offset, top: offset })
}).then(([img1, img2, img3]) => {
    canvas.add(new fabric.Group([ img1, img2, img3], { left: 200, top: 200 }))
});

(Of course it doesn't make much sense to load the same image three times like this, I'm just translating the tutorial example verbatim for demonstration purposes.)

Reasoning for the feature request: All the usual benefits of Promises; easier composition, more reliable error handling, no chance of accidental double callbacks, and so on, and so forth.

Implementation and consequences: This does not need to be a breaking change; an approach similar to Bluebird's nodeify/asCallback could be taken, where a Promise is returned by default, and a callback is only called if one is specified.

I would not recommend actually using Bluebird in this case, for two reasons:

  1. Bundle size. Bluebird is relatively big, and it probably doesn't make sense to throw in all of Bluebird for this relatively small change, especially given that fabric.js is browser-oriented.
  2. Bluebird's .asCallback assumes an error-first callback, but fromURL doesn't use those; there is no first errror argument.

As for browser support; there are roughly two options here:

  1. Include an ES6 Promises shim bundled into fabric.js.
  2. Make the "nodeification" logic dependent on whether there is an ES6 Promise implementation available in the environment; if yes, return a Promise, if no, return nothing. This puts the burden on the user to polyfill global.Promise if they want to use Promises in all environments, without affecting non-Promise-using users.

The second option is probably preferable from a bundle size and "interference with other users" point of view, but is a bit more complex implementation-wise.

I'd say that overall implementation complexity and work required for somebody who knows the fabric.js codebase would be very low, since it's a generalizable conversion - but one that is much easier and more reliable to do from within the library than on the outside. Unfortunately I don't really know much about the fabric.js codebase, or I would've outright submitted a PR...

(Of course, I'd be happy to answer any questions about the Promises aspect of this, to make implementation go smoother; implementation details, common usage patterns, and so on.)

EDIT: Fixed example to actually be equivalent. More caffeine is required...

@asturur

This comment has been minimized.

Copy link
Member

asturur commented Feb 11, 2017

so this has been already discussed and there is an abandoned or somewhere.

it will be done, with the approach:

  • promise default or callback
  • if you do not have promise in the browser solve it by yourself

i did not check all asyncronous functions yet and found possible problematic spots

@joepie91

This comment has been minimized.

Copy link

joepie91 commented Feb 11, 2017

Hmm. I searched the issue tracker before posting, and I couldn't find anything. Do you have a link to the previous proposal/issue/PR for me?

@asturur

This comment has been minimized.

Copy link
Member

asturur commented Feb 11, 2017

#2556

the discussion was a generic possible es6 migration that for now i m pushing forward since the average fabric dev use fabric in simple environments, and i have no time to mess up with babel for now in my free time

@joepie91

This comment has been minimized.

Copy link

joepie91 commented Feb 11, 2017

Ah, I see. That seems to be a more involved effort to actually change the code to use Promises internally as well (as opposed to just exposing them on the public API).

I do see a few issues with the other PR. Would you prefer I leave some review notes on there, make a proposal for a new (exposed-API-surface-only) implementation, or both?

(The benefit of an exposed-API-surface-only implementation would be that, for now, you can support Promises without needing to set up Babel or adding in a polyfill yourself.)

@asturur

This comment has been minimized.

Copy link
Member

asturur commented Feb 11, 2017

better here

@joepie91

This comment has been minimized.

Copy link

joepie91 commented Feb 12, 2017

Alright. I've written a wrapper implementation this morning that should be suitable for adding provisional Promise support, until a 'proper' code conversion rolls around. If it works as intended, it won't affect the current workings of callbacks (nor will it change their signature), and it should only involve a minimal code change in fabric.js itself.

I still need to test it out in a few strange edge cases, though, and I might not get around to finishing that today. If not, I'll finish the testing tomorrow, and post my proposal here then.

@asturur

This comment has been minimized.

Copy link
Member

asturur commented Feb 12, 2017

thanks. is mainly a question of code style and code quantity.
i ll release 1.7.4 soon and close the 1.x branch and then start those changes.

@asturur asturur changed the title Feature request: Promises support Promises support Feb 19, 2017

@asturur asturur added the feature label Feb 19, 2017

@asturur asturur added this to the 2.0.0 milestone Feb 19, 2017

@asturur asturur modified the milestones: 2.0.0, 2.1.0 Jul 2, 2017

@p3ndu

This comment has been minimized.

Copy link

p3ndu commented Sep 22, 2017

Hi,

Any update on this, when could we expect it?

Regards,
Alex

@c-gross

This comment has been minimized.

Copy link

c-gross commented Dec 18, 2017

I think await would be a better way. It's just cleaner code.

@asturur

This comment has been minimized.

Copy link
Member

asturur commented Dec 18, 2017

Well a Promise polyfill is fewer lines of code, and other than that is not just cleaner code, is a different behaviour completely.

@c-gross

This comment has been minimized.

Copy link

c-gross commented Dec 18, 2017

Yes it is. I remembered a video I watch some time ago when I checked a few things for my next project.

https://www.youtube.com/watch?v=NsQ2QIrQShU

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment