4 ms·
https://github.com/CapacitorSet/rebridge/blob/master/index.js https://github.com/CapacitorSet/rebridge/blob/master/index.j... A quick review... 1. Could have
by partycoder 10y ago
https://github.com/CapacitorSet/rebridge/blob/master/index.js https://github.com/CapacitorSet/rebridge/blob/master/index.j...
A quick review...
1. Could have made your life easier using promises or equivalent to handle the asynchronous flows.
2. Could have made your life easier using lodash instead of implementing recursive functions that can throw stack overflows.
3. JSON.parse can throw with invalid JSON and JSON.stringify can throw with circular references. There might be other cases in which those throw.
4. Line 25: "if key is inspect, return thing". That wasn't in test code, but in the actual library code. That would execute in the application using this library.
...And that part was the one that pushed me over the edge and the extent of my review. I punched my keyboard.
Then, you claim it handles a redis connection transparently. But nobody would use a redis server without redundancy, you need a redis sentinel somehow, and this does not support it. When you have a sentinel you need to handle failover events. The library does none of this, and is therefore not suitable for production use.
- stephen_mcd 10y agoYou're not wrong, Walter.
- partycoder 10y agoA bug in a library like this is not a joke.
- nkohari 10y agoSure, but OP didn't specifically ask you to review the code anyway, and there's a way to give feedback that doesn't make you look like an asshole.
- stephenr 10y ago> Sure, but OP didn't specifically ask you to review the code anyway, Posting to Show HN isn't asking for review?
- partycoder 10y agoRight now all other feedback I am seeing is exactly like this: https://xkcd.com/937/ https://xkcd.com/937/
- partycoder 10y agoIt's open source, you provide code so people contribute by reporting issues or contributing code, documentation, artwork, etc. So no. Then, collaboration is saying what you need to hear, not what you want to hear. If you are not prepared to receive feedback, try another activity not open source. Then, why did I say a bug in a library like this is no joke? From experience. Some years ago I was asked to solve a production incident that was caused by a redis wrapper similar to this one. After it was fixed, the impact was estimated to be $1 million dollars, which caused a project to be canceled and the entire team to be laid off. Those people had absolutely no responsibility in the incident.
- CapacitorSet 10y agoThank you for your feedback. * I don't think the asynchronous flow can be further simplified. If you do find a sleek solution, do submit a PR, or even an issue with some sample code. * Stack overflow is not an issue at this time (and even less in the future, with tail call optimization in V8), except for pathological objects. As such, I didn't use lodash, to keep the library as small as possible. * JSON.parse errors are not an issue, under the assumption that the Redis database is only written to by Rebridge itself. If you find otherwise, do submit an issue. Circular references are indeed a known limitation, documented in the README. * Thanks, I removed it in beda881. It was probably a debug line. For the sentinel, I haven't yet had to use it (I used it in a small Telegram bot with a single Redis server on a cheap VPS). I'll open an issue for that.
- partycoder 10y agoYou can promisify redis during initialization and then use redis operations using promises. You can still handle exceptions thrown by JSON.parse/stringify functions. Otherwise you can document that your functions are going to throw. (jsdoc: @throws) You cannot assume a specific redis setup. If there's an intended redis setup then specify it in the README file. Add an inline comment around your debugging code, e.g: (// FIXME: Do not check-in), so you can remove it while double-checking what you are committing.