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

Promises support #161

Closed
wants to merge 6 commits into from
Closed

Promises support #161

wants to merge 6 commits into from

Conversation

catalint
Copy link

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

  • the callback was binded to process.domain, with promises I could't find an analog, and from what I've seen node domain is deprecated, my best guess is that using promises doesn't need the use case callback needed when binding
  • Promise.resolve() accepts only one argument, so:
    • policy.get(key))
    • it resolved the cached / generated value
    • if a stale timeout or any error that doesn't invalidate the cache occures, the cached item is sent via resolve()
    • if any other unrecoverable error , it calls reject(err)
    • policy.get(key,{full:true})
      • it resolved an object with the previously sent arguments (err, value, cached, report)
      • if err is present and not undefined/null , then it it rejected with the same object, even if the cached value may be still valid)

todos

  • hapi support for this in other repos
  • a major version increment will be needed

what are your thoughts on this?

@catalint
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

We can have both worlds!

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

@catalint
Copy link
Author

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
Copy link
Contributor

Marsup commented Apr 20, 2016

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

@hueniverse
Copy link
Contributor

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

@catalint
Copy link
Author

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
Copy link
Contributor

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 hueniverse added the feature New functionality or improvement label Apr 25, 2016
@hueniverse hueniverse self-assigned this Apr 25, 2016
@hueniverse
Copy link
Contributor

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.

@hueniverse hueniverse closed this Aug 26, 2016
@einnjo
Copy link

einnjo commented Aug 7, 2017

Can this be reconsidered now that hapijs/hoek#176 met a dead end?

This might also help with: hapijs/hapi#3150

@lock
Copy link

lock bot commented Jan 9, 2020

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants