Ankur's feedback

[ggarber]

feedback for SnapFramework

• log4js still has to be required outside of the module? weird.
• HttpClient[method] sucks
• instrument sucks because the signature for f is very strict. why is this not in Snap?

[ggarber]

• log4js require is known issue I agree but i don't have a great solution yet..
• why? any better idea to wrap request or a similar library? (apart from the fact that all the snap modules should expose an API based on promises)
• It could be in Snap, i'm not sure yet though. Snap should be more a framework than a utility library. Regarding "signature is very strict" you mean that it assumes that it is an asynchronous function with the last parameter being a callback? I don't think it is that strict, specially if it is not part of Snap. I think it saves lot of code/issues in id-server. What is the alternative? I would love to have a db-client in snap doing all of that automatically :)

[aoberoi]

why? any better idea to wrap request or a similar library? (apart from the fact that all the snap modules should expose an API based on promises)

it wasn't clear that it would work (because of the implementation in id-server) but i wanted to pass method in the request options object, rather than using request[method](options). now that i see there's no modification to the request API, i understand i can do this. the README says "standard request module for node.js", and that was confusing, because this module is not standard, its from npm. there are many other libraries people use (eg. superagent, etc) for a similar purpose, and long term this API won't be preferred because as you said we want Promises.

[aoberoi]

It could be in Snap, i'm not sure yet though.

i read the code wrong. i thought it only worked with async functions with the first argument being the only real argument and the second argument must be the callback. this made me think that all instrumented functions need to put all their arguments into an options object, and use the second argument as the callback. i see thats not the case now. but instrument will only log the first argument, and i think that can be improved. i now see there's the stats.track() method in the stats module that does the same thing as instrument, why wasn't that used in id-server?