honza / node-thumbnail

Thumbnail worker queue for node.js
http://honza.ca/node-thumbnail/
Other
103 stars 23 forks source link

Add basic support for native Promises #16

Closed oksas closed 7 years ago

oksas commented 7 years ago

This is my stab at implementing Promises so that this module supports both Node-style callbacks and Promises. Here's a rundown of the changes:

Let me know how that looks! Definitely open to feedback here, and I'm not 100% about everything. I think this might be a good stage to add some simple tests to the module, to always ensure both callbacks and Promises work, reject with errors at the right time with insufficient or invalid input, etc. I have a small ugly script I have been using to test this stuff myself but it might be worth adding some tests to the module itself.

Closes #11

oksas commented 7 years ago

O silly me, didn't see that you did actually add unit tests. I will look into the failing test here then and fix that.

Edit: Oh hah looks like the test is meant to fail

honza commented 7 years ago

At first glance, this looks good. I'll pull down the changes later, and take it for a spin. Thanks for submitting this!

oksas commented 7 years ago

Sounds great, thanks dude!

honza commented 7 years ago

Running ./bin/thumb without any arguments throws this error:

(node:28912) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): TypeError: options.logger is not a function

Other than that, works great!

oksas commented 7 years ago

Hmm ok, glad you tested the CLI because I clearly did not. I'll look into it this afternoon and refine. Thanks man!

oksas commented 7 years ago

It should be all good to go now. I ran my usual tests for ensuring the module works still with both callbacks and Promises, and the command line interface also seems to work without issue. Let me know how it looks!

honza commented 7 years ago

Very good, thanks ✨