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

Promise support #2323

Closed
miguelcobain opened this issue Dec 24, 2014 · 6 comments
Closed

Promise support #2323

miguelcobain opened this issue Dec 24, 2014 · 6 comments
Assignees
Labels
Milestone

Comments

@miguelcobain
Copy link

@miguelcobain miguelcobain commented Dec 24, 2014

Would it be appropriate to change this line in response.js from:

this.source.then(onDone, onDone).catch(onDone);

to

this.source.then(onDone, onDone).then(null, onDone);

The reason for this is that some promise implementations still don't have catch implemented.
A major example is Mongoose/mpromise.
Open to discussion.

@Marsup
Copy link
Contributor

@Marsup Marsup commented Dec 25, 2014

Maybe the catch shouldn't be there in the first place, unless onDone has a chance to fail and can recover with a second call to itself.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Dec 28, 2014

I have no clue. I don't use promises. I'll let @Marsup make the call on this one.

@Marsup
Copy link
Contributor

@Marsup Marsup commented Dec 28, 2014

@hueniverse Is the last part of my sentence possible?

@bendrucker
Copy link
Contributor

@bendrucker bendrucker commented Dec 28, 2014

I can't see a way that it would throw such that a second call would resolve it

@Marsup
Copy link
Contributor

@Marsup Marsup commented Dec 28, 2014

Then just removing the catch part should do it.

bendrucker added a commit to bendrucker/hapi that referenced this issue Dec 28, 2014
Adds compatibility for promises that implement the bare A+ spec

Closes hapijs#2323
@bendrucker
Copy link
Contributor

@bendrucker bendrucker commented Dec 28, 2014

Fix ready to merge in #2324. I don't think it merits a test but I can't certainly add one if desired.

@hueniverse hueniverse added bug and removed support labels Jan 3, 2015
@hueniverse hueniverse self-assigned this Jan 3, 2015
bendrucker added a commit to bendrucker/hapi that referenced this issue Jan 4, 2015
Adds compatibility for promises that implement the bare A+ spec

Closes hapijs#2323
@hueniverse hueniverse added this to the 8.0.1 milestone Jan 5, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants