search

nknapp / promised-handlebars

Wrapper for Handlebars that allows helpers returning promises
MIT License
48 stars 14 forks source link

Use bluebird instead of Q to get a better performance. #13

Closed anson0370 closed 8 years ago

anson0370 commented 8 years ago

I have done this on my fork. And I got a really performance improve. I'm not sure is there any problem with this change so I didn't make a PR.

Thanks for your work.

nknapp commented 8 years ago

I've preferred Q at the moment for the smaller library size (i.e. download time). Bluebird seems to carry a lot of stuff that is not needed in node.

But, if it a performance improvement, I would consider changing this. For compatibilty reasons, I would add an option to the main function where the Promise constructor can be passed in. Apart from the ".all" function, ".isPromiseAlike" and "deep", no special functions are used, so this might even work with a plain ES6-Promise constructor and without any libraries.

I would remove the dependency to q only with a major version bump. If you want to make a PR for discussion, please go ahead. I should be based on the current master though (there will be another commit shortly, so you may want to wait until the end of the week. I still have a bug to fix).

anson0370 commented 8 years ago

Give an option to allow users to pass in the Promise implementation what they want is make sense. .all function is a part of plain ES6-Promise, we only need to implement the .isPromiseAlike function which is simple. So yes I think this should work with the plain ES6-Promise.

The performance improvement is really obvious in my case. Maybe I can post some data here this week later or next week (busy work this week 😢). And I noticed you just refactored your code. I'll try the new version after you finish the bug fix.

Thanks for create this package again. It's really helped. 😄

nknapp commented 8 years ago

I created this package, becaues I needed it in Thought. Nice to hear that it helped you.

I have released my fix (#14) in promised-handlebars@1.0.6. Unless there are other bugs coming in, I won't be doing any development on the repo at the moment, so you can get started, if you want...

jskrzypek commented 8 years ago

I'll take a stab at upgrading the lib to remove the need for the Q dependency.

jskrzypek commented 8 years ago

I made a PR that is able to replace q with other promise libs. I added test setups for bluebird and es6-promise as well to make sure it works.

anson0370 commented 8 years ago

@jskrzypek U r awesome. #20 Just make a link.

nknapp commented 8 years ago

20 is merged now