-
-
Notifications
You must be signed in to change notification settings - Fork 37
Support for Promises #16
Comments
Yes, we would love to get PRs for promise support! cc @g0ddish |
Here's my first crack at a promised based PDFMerge...needs to be thoroughly tested! |
Hmm I think its fine for now though its just an improvement. |
@ksumarine Thanks for spending time on adding promise support. I just checked it out. Could you raise a PR with this code? I just have one comment for now, I see that there's only Promise support in the code that you posted, this will break code for people already using the library. To tackle this, we can either release a major version change. Or we could support both callbacks and promises. I would like to choose the latter. I remember seeing some libraries support this by doing - if callback func is passed, then use that, if not return promises. Also, one more thing I noticed is you have used the default opts value in the function parameter definition, that's great. Just one thing, the author who wrote the code for opts has also considered the case where people call the function with only source files array, destination and callback (as 3rd arg) with no opts, the 3rd arg was considered as callback, and then used default opts. Could you consider these cases by looking at the current code? 😄 I would be happy to help if you have any questions regarding this |
@karuppiah7890 you got it! Sorry for the delay, I'm at work. I can try to work on this tonight and attempt a PR (this would be my first for someone else's project). You raise good concerns that I hadn't considered when I tried the promises. I will do my best to account for those situations. Please let me know if I didn't get it right or please send suggestions if you have any. I appreciate your help! |
Hello bro, If you don't mind I used hummus pdf as it required something like apache pdf box and turns out it also worked really well without using any dependencies. Can u check that too may be it will be more useful too you ? Here is how i merged using hummus js
|
Please don't mind extra code as i wrote it for my own purpose :) |
@shirshak55 thanks for the info. I will check out the package 😄 |
Seems a common strategy for this is to return a promise if no callback function is supplied. Example (
It's slightly obfuscated in the code but it still serves as an example. |
I think promises are better now isn't it?
The text was updated successfully, but these errors were encountered: