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

Switch from callbacks to promises for findRoad #2

Merged
merged 2 commits into from Nov 10, 2015

Conversation

@JakeChampion
Copy link

commented Nov 7, 2015

Callbacks are useful when an async action may fire multiple times, promises are useful when an async action only fires once. They also benefit by not inverting the control, which is what callbacks do.

Switch from callbacks to promises for findRoad
callbacks are useful when an async action may fire multiple times, promises are useful when an async action only fires once. They also benefit by not inverting the control, which is what callbacks do.
@JakeChampion

This comment has been minimized.

Copy link
Author

commented Nov 7, 2015

This has changed the signature of findRoad, the consumer on this function would have to be updated to consume promises instead of passing a callback.

@aksiksi

This comment has been minimized.

Copy link
Owner

commented Nov 8, 2015

Thanks for the changes! I've merged them into the dev branch, but I think I'm handling the returned Promise from findRoad incorrectly. What's confusing is that the first Promise can return either a response object or another Promise. How can that be handled?

var findRoad = (parsed, db, callback) => {
const resp = newResponse();

const findRoad = ({lat, lng}, db) => {

This comment has been minimized.

Copy link
@JakeChampion

JakeChampion Nov 8, 2015

Author

findRoad should only be returning a Promise wrapped Response object. I'll comment on the lines with explanations.

return findOneSegment(q, {speed: 1, road_id: 1, _id: 0})
.then({road_id, speed} => {
if (!segment) {
return resp;

This comment has been minimized.

Copy link
@JakeChampion

JakeChampion Nov 8, 2015

Author

Returning anything from a thenable will wrap it in a Promise, this enables chaining and error propagation to the consumer.

This return statement is called when findOneSegment executes successfully but does not find a segment.

})
.catch(err => {
resp.online = 0;
return resp;

This comment has been minimized.

Copy link
@JakeChampion

JakeChampion Nov 8, 2015

Author

This return statement is called when findOneSegment executes unsuccessfully.

});
const roads = db.collection(config.roads);
const findOneRoad = denodeifyMethod(roads, 'findOne');
return findOneRoad({_id: road_id})

This comment has been minimized.

Copy link
@JakeChampion

JakeChampion Nov 8, 2015

Author

This is returning a Promise, however the promise already has some thenables chained onto the end. The return of last thenable or the return of the catch (if a catch is attached) is what gets returned to the consumer of this findRoad.

resp.found = 1;
resp.speed = speed;
resp.name = road.name;
return resp;

This comment has been minimized.

Copy link
@JakeChampion

JakeChampion Nov 8, 2015

Author

This return statement is reached if a segment and a road are found. The Response object resp is returned to the consumer of findRoad.

})
.catch(err => {
resp.name = "";
return resp;

This comment has been minimized.

Copy link
@JakeChampion

JakeChampion Nov 8, 2015

Author

This return statement is reached if a segment is found but an error was thrown when trying to find a road. The Response object resp is returned to the consumer of findRoad.

@JakeChampion

This comment has been minimized.

Copy link
Author

commented Nov 8, 2015

If you haven't used Promises a lot, Nolan Lawson has a written quite a good blog post on the topic, it includes a short quiz :-) - http://pouchdb.com/2015/05/18/we-have-a-problem-with-promises.html

@aksiksi aksiksi merged commit e5a69c1 into aksiksi:master Nov 10, 2015

@aksiksi

This comment has been minimized.

Copy link
Owner

commented Nov 10, 2015

It seems there was a slight error which I fixed in the latest commit. Thanks for your contribution!

By the way, you are not appearing in the Contributors graph because you haven't added the email you used to commit to Github.

@JakeChampion JakeChampion deleted the JakeChampion:patch-1 branch Nov 10, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.