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

Minor improvements considerations, Error handling and quality. #1

Open
7 tasks done
SHAcollision opened this issue Apr 23, 2024 · 0 comments
Open
7 tasks done

Comments

@SHAcollision
Copy link
Contributor

SHAcollision commented Apr 23, 2024

Hey @dzdidi, really nice work, congratz! I took a short look over the code base and wrote down a few minor items as I was going for things we might (or not!) consider to improve.

  • It depends on how purist and "well made" we want this crate to be. But only on client.rs we have 24 .unwrap() instances. Ideally we handle the errors gracefully using match or the ? operator to propagate errors up the call stack. If we are going to wrap this lib as an NPM package with wasm-pack the errors will be returned to the Javascript layer gracefully. Many of the .unwraps() are chained, so fixing these might result in somewhat more verbose code.
  • Add comments and documentation, especially for public methods and struct fields.
  • The Client struct and its methods are generally well-structured, but there's room for improvement in adhering to Rust idioms. For example, using more match statements helps leveraging Rust's type system and performance (e.g., all of those unwraps).
  • Again depends on how "purist" we want to be: we use lifetimes in the Client struct definition, but it's not clear how these lifetimes are used or why they are necessary. Ensuring that lifetimes are used correctly and only when necessary is important for memory safety and performance.
  • The version path is hardcoded ("/mvp/...."). We might want to be able to provide a version param on client instantiation client::new() .
  • This assertion
    https://github.com/slashtags/pubky_homeserver_client/blob/19a53cb4db46b51f9a2e33fbc663ed2263937cbe/src/client.rs#L336
    will always succeed since this is a comparison of unit values (unless .create() errors, but then the assertion won't run). Well, in any case, you figure if the thing is broken...
  • This repo name uses lower bar _ as separator, while most other repos in our orgs use dashes - . Names makes no difference to me, but very clear and self-explanatory names following a decent naming convention will help a ton future developers joining when public. A good naming convention, given pubky-app being the web UI, would be to name the Rust reimplementations of pubky-core-js (skunk-works) by using the prefix pubky-core-... . This way we will have a pubky-core-homeserver (future work) and this repo pubky-core-client.

HTTP Requests in WASM

  • AFAIK directly making HTTP requests from WASM running in a browser is not straightforward due to the sandboxed environment of WASM. WASM does not have direct access to network resources or the host's operating system features, including making HTTP requests. That said, we should take inspiration from https://github.com/MutinyWallet/mutiny-node/tree/master/mutiny-wasm , these crates do use reqwest so there must be something I don't know.
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

No branches or pull requests

1 participant