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

Update README.md #124

Merged
merged 2 commits into from
Sep 30, 2018
Merged

Update README.md #124

merged 2 commits into from
Sep 30, 2018

Conversation

roblabat
Copy link
Contributor

Update documentation on Responder.on callback (the callback need to be call as cb(error, result) instead of cb(result) as used in https://github.com/dashersw/cote-workshop

Update documentation on Responder.on callback (the callback need to be call as cb(error, result) instead of cb(result) as used in https://github.com/dashersw/cote-workshop
@coveralls
Copy link

coveralls commented Sep 24, 2018

Coverage Status

Coverage decreased (-0.3%) to 93.712% when pulling 5c4b26f on roblabat:patch-1 into cf0f96d on dashersw:master.

@dashersw
Copy link
Owner

I am a little ambivalent about this PR — because technically you don't need to use a signature of cb(error, value) if you are not mixing promises or async/await with callbacks. If you are only using callbacks, or if you are only using promises and or async/await, you don't run into this problem.

@roblabat
Copy link
Contributor Author

You're true this perfectly works if we aren't combining both approaches (async/await and callbacks). But as both approaches are supposed to work the same way I think it's better to use a signature that is compatible with both. Currently using the signature cb(value) is like using the error channel to passe some data and even if it works perfectly I don't think it's the best practice.

The most important point on having a unified signature that allows to using promise written services with callbacks ones is that your package is perfect for a microservice approach. In that approach we want to be able to separate works on different microservices to different teams and those different teams could prefer one or the other approach to write their service and the signature cb(error, value) allows that.

@dashersw
Copy link
Owner

dashersw commented Sep 25, 2018

I agree. Therefore I believe this deserves a title in the README, and is documented in detail. I certainly don't want to force an error first style on everyone, so the README shouldn't present it as the only / default way, but should more elaborate on it.

@roblabat
Copy link
Contributor Author

I think not presenting it has a default way could create some compatibility uses if someone wants to build an opensource microservice based on cote. Then he should chose a signature rather than the other and his project will only be compatible to projects matching his choice. I also understand that forcing people using an error first signature is not perfect as if you do not want to handle errors you are forced to pass a null to the callback every time. For that reason I believe that a compatible signature must be promoted by the package to allow the community to create ready to deploy services for all cote projects.

I don't know if it's possible but maybe an update in the package could invert the error and the value arguments if the callback passing the error has the second argument and then making both signature cb(value) and cb(value, error) compatible with promises ?

@dashersw
Copy link
Owner

We in fact use a single argument object for the requests anyway — theoretically, if there are two arguments then the first one may be treated as an error.

@dashersw
Copy link
Owner

@cakuki @pelzerim can you look into this?

@dashersw
Copy link
Owner

@roblabat We looked into the feasibility of changing the code, but currently there are no practical umbrella solutions. The best way to move forward therefore is just to update the readme and always present a two parameter style. But there's one more line, 601, where there's a callback you need to add null. Could you add it?

@matejthetree
Copy link
Contributor

I agree on this one, since I ran into troubles as well when I started on cote. My observation is that most newcomers will try to use cote with promises and async/await. There for it would be good idea to modernise the examples and README

@dashersw dashersw merged commit a533dc3 into dashersw:master Sep 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants