search

knope-dev / knope

A command line tool to to handle all the tasks most developers find tedious.
https://knope.tech
MIT License
88 stars 8 forks source link

Replace octocrab #68

Closed dbanty closed 3 years ago

dbanty commented 3 years ago

This dependency binds us to specific Tokio/Reqwest versions. I'd rather use async-std anyway, and we don't have that option. It also contains a ton of features we don't use.

The immediate solution would be to write our own requests for the few pieces of the GitHub API that we do use, although we need to weigh this against the possibility of adding more direct GitHub integrations.

We could also fork octocrab and make it compatible with async-std using a feature flag (if portability is the main concern).

dbanty commented 3 years ago

Also I think this is the only reason we use async, we don't do anything in parallel. And reqwest+octocrab+Tokio are > 25% of Dobby's size so that doesn't seem worth it.

So the real answer is probably to rewrite with blocking code and take out async altogether.

Shadow53 commented 3 years ago

On the topic of blocking code, both ureq and attohttpc are both blocking HTTP clients that provide JSON support via serde. I think a rewrite using blocking calls would be served well using either one, based on brief glances at both.

dbanty commented 3 years ago

Some additional info pre-refactor:

  1. cargo build --release takes about 2 minutes to complete after a cargo clean on my machine. Probably significantly longer on the average machine / internet connection.
  2. The built dobby is 7.8MB

Hopefully those numbers go down considerably with the dependency simplification.

dbanty commented 3 years ago

With switching out octocat/reqwest for ureq:

  1. Build time is down to 1.5 minutes (25% savings!)
  2. Binary is down to 6.4MB (~18% savings!)

Seems like a pretty clear win. I think the GitHub code is actually a little cleaner now too, as previously it was fetching both issues and PRs and filtering client-side. Now we're fetching just Issues because we're using the new v4 API (I assume, based on the limitation, that Octocrab was using v3).

According to cargo bloat the next biggest area we might get savings is from ditching regex. We only use it right now to parse branch names so it should be easy enough to replace with std string functions. Looks like toml_edit is pretty big too. We could replace that with standard toml (which we also have and need), it'll just take more doing. I'll create separate issues to explore those things.