hapijs / catbox

Multi-strategy object caching service
Other
494 stars 72 forks source link

Promises support #161

Closed catalint closed 8 years ago

catalint commented 8 years ago

An attempt at removing callbacks and relying on promises.

No major refactoring was needed for tests, just removing callbacks & relying on promise chain

notable changes for client

none

notable changes for policy

what are your thoughts on this?

catalint commented 8 years ago

I'll change the tests a bit to better handle promises, will need to upgrade to lab 10 (it's 8 right now in package.json)

hueniverse commented 8 years ago

No chance I will be removing callbacks. I am open to offering a secondary promises support same way I did in hapi. That is, callback is the default and how the system is designed while promises support is added as syntactic sugar.

Note that I am not anti-promises or am passing judgement on people who likes to use them. It's a perfectly valid design choice. It's just not my design choice.

catalint commented 8 years ago

We can have both worlds!

Expect this PR to be updated in a couple of days.

catalint commented 8 years ago

updated to support both promises & callbacks, no change in logic was done to callbacks , callback tests ran without any changes

added tests for promises , added promise sections in README.MD after the callback ones

Marsup commented 8 years ago

Maybe it's time we re-discuss putting https://github.com/hapijs/hapi/blob/master/lib/promises.js into hoek.

hueniverse commented 8 years ago

@Marsup someone should just do it and PR the move on hapi.

catalint commented 8 years ago

hi @hueniverse have you gotten the chance to look over this? if you require any changes I'm more than happy to do them. thanks!

hueniverse commented 8 years ago

I'd like to see the pattern used by hapi moved to hoek and then used here. Also, do we really need to c/p the entire readme?

hueniverse commented 8 years ago

As I said back in April, the only way I am going to support promises is using the same pattern I used in hapi. This would require moving that utility to hoek. I might look into that but it's low on my list.

einnjo commented 7 years ago

Can this be reconsidered now that https://github.com/hapijs/hoek/issues/176 met a dead end?

This might also help with: https://github.com/hapijs/hapi/issues/3150

lock[bot] commented 4 years ago

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.