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

Promisify Functions #486

Merged
merged 8 commits into from
Jul 30, 2018
Merged

Promisify Functions #486

merged 8 commits into from
Jul 30, 2018

Conversation

hipstersmoothie
Copy link
Collaborator

closes #232 - write now can return a promise
closes #90 - same as above and getBuffer can now be async
closes #374 - base64 can now be async
closes #308 closes #445 - Jimp.read can take all args that the constructor can. Jimp.create added for clearer syntax. No need to add promises to the jimp constructor.

@hipstersmoothie
Copy link
Collaborator Author

@oliver-moran If you just merge this one all #473 and #477 will be merged as well

@hipstersmoothie
Copy link
Collaborator Author

@oliver-moran and if this one gets merged 10 issues get closed all at once 😮

@hipstersmoothie
Copy link
Collaborator Author

hipstersmoothie commented Jul 27, 2018

I think I'm gonna change it from getBuffer(mime, 'async') to getBufferAsync(mime)

@hipstersmoothie
Copy link
Collaborator Author

@oliver-moran Just added the 'async' functions. They are tested and documented. Everythings runs fine. This should be ready to go!

@hipstersmoothie
Copy link
Collaborator Author

@edi9999 what do you think of this one? it gets makes of the requested functions promises without breaking the current api

@edi9999
Copy link
Contributor

edi9999 commented Jul 28, 2018

in this MR I find that the duplication in the code is not good for maintanability.

Things like Bluebird can do the automatic transformation from callback to Promises.

@hipstersmoothie
Copy link
Collaborator Author

So should I use bluebird to do this? Or you want users to do that? I think it would be better just to have it supported. The code isn’t really duplicated either. I factored out the main functionality of each function and the sync and a sync functions just call them in different ways

@hipstersmoothie
Copy link
Collaborator Author

@edi9999 Switched to an implementation that just calls the current functions and return a promise. Similar to Jimp.read. I believe we should provide this functionality and not make the user do it themselves. Since we return promises in a few other places, it makes sense to do it in the last few places we can.

@hipstersmoothie
Copy link
Collaborator Author

@edi9999 have i fixed your duplication concerns?

@edi9999
Copy link
Contributor

edi9999 commented Jul 30, 2018

You can even just do it like this :

const promisify = (fun, ctx, args) => {
  return new Promise((resolve, reject) => {
    args.push((err, data) => {
      err && reject(err);
      resolve(data);
    });
    fun.apply(ctx, args);
  });
};

 writeAsync(path) {
        return promisify(this.write, this, [path]);
 }

This way the "promisification code" is just in one place.

@hipstersmoothie
Copy link
Collaborator Author

@edi9999 added promisify util function

@edi9999 edi9999 merged commit f19e77f into jimp-dev:master Jul 30, 2018
@hipstersmoothie hipstersmoothie deleted the promise2 branch July 30, 2018 09:20
@favna
Copy link
Contributor

favna commented Jul 30, 2018

I see this closes #445 and some talk about Jimp.create but I can't find any documentation on this new function nor (easily) find it in the code changes by the PR. Could you clarify the usage?

@hipstersmoothie
Copy link
Collaborator Author

hipstersmoothie commented Jul 30, 2018

You can just use Jimp.read to make a Jimp instance. It takes all the same args as the constructor. Jimp.create also exists but it just calls Jimp.read. I'll document it now

https://github.com/oliver-moran/jimp/blob/master/src/index.js#L810

@septs
Copy link

septs commented Aug 2, 2018

Request update "jimp.d.ts" file defines.

@hipstersmoothie
Copy link
Collaborator Author

@septs what updates are you requesting ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants