mirror of
https://github.com/ramsayleung/rspotify.git
synced 2026-04-26 07:55:55 +03:00
[GH-ISSUE #108] Reducing rspotify's core dependencies #36
Labels
No labels
Stale
bug
discussion
enhancement
good first issue
good first issue
help wanted
pull-request
question
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/rspotify#36
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @marioortizmanero on GitHub (Aug 12, 2020).
Original GitHub issue: https://github.com/ramsayleung/rspotify/issues/108
I've been creating various PRs to clean up some unnecessary dependencies I found and such. Not only does this decrease compilation times, but this may also reduce binary size in case the optimizer is unable to know a module isn't used at all.
Here are some of my proposals, don't hesitate on criticizing them; I want to discuss these changes with @ramsayleung and if possible with more users of rspotify. It might also be too much to remove some of them:
Optional features
webbrowseroptional, under someclifeature. This dependency isn't used by all the users, since some may not need to open a browser for the authentication process. It's only used for theutil::request_tokenfunction currently.dotenvoptional, under someenvfeature. This is also only used by some users, mostly web servers.These changes will break backward compatibility, so a new version would have to be released. I could create an
UPGRADING.mdfile that explains what changes were made in order to make this process easier.Removing dependencies
cargo run --example current_playingto 177, so ~14 less dependencies after #105lazy_staticis only used inclient.rsforCLIENThere. ThisCLIENTvariable is then only used once here. I haven't usedlazy_staticmyself, but I wanted to re-evaluate the following questions:- Is it necessary that
CLIENTis a global, static, public variable?- Is
lazy_staticis necessary here?randnecessary only forutil::generate_random_stringhere?randis a considerably big dependency because it includes lots of things that are often unnecessary. I recently saw this PR https://github.com/rust-analyzer/rust-analyzer/pull/5574 that replaced it withoorandom, a much simpler and straightforward crate. Could this also be applied to this use case?@ramsayleung commented on GitHub (Aug 13, 2020):
It's a great suggestion, which makes it more easier to build automatic tests without
webbrowser. But when we need credential to call Spotify's endpoints, how to get credential without pulling up a browser? It's the original reason of why I usedwebbrowser.Yes, I originally use
dotenvto parse environment variable likeCLIENT_ID,CLIENT_SECRET. It will be fine if users prefer pass those parameters directly, so we could just makedotenvoptional.Honestly, it's the only usage of
rand, just generating a random string. If we could find a lightweight replacement(or could we implement thegenerate_random_stringwithout any additional crate?), it's great to removerandto reduce compile time and binary size.If it's the best choice for us, why not?
@marioortizmanero commented on GitHub (Aug 13, 2020):
My point is that opening a new browser window/tab is only needed for those creating CLI applications:
util::request_tokenbecause you need a more user-friendly authentication process within a GUI (my case, where I run a small HTTP server to have an user-friendly and automatic redirect uri),webbrowserisn't needed for rspotify, only for your binary.As long as it's well documented I think it will be fine.
Great. I'll try to come up with something for that and I'll open a new PR once I have it ready.
Do you want me to create these features in
Cargo.tomland the documentation regarding them in a new PR, or do you prefer to do it yourself? In casewebbrowseris made optional, most examples will have to be changed as well. Also the tests fordotenv.@ramsayleung commented on GitHub (Aug 13, 2020):
I prefer to open a PR to implement this reduction, we both could commit to this PR. And now I am trying to replace
randwithoorand, it's funny. I can't help to think, how much improvment will this reduction make.@marioortizmanero commented on GitHub (Aug 13, 2020):
I just noticed
oorandompoints out in their README.md that it's not cryptographically secure. Knowing thatget_random_stringis used for thestateparameter in the authentication process. From the OAuth 2.0 specification:So I don't think removing
randis worth it. That, or finding another lightweight random number generator that is cryptographically secure, like thegetrandomcrate.@ramsayleung commented on GitHub (Aug 13, 2020):
I just create a code snippet with
getrandomto generate a random string:it works.
in
getrandom's README, I noticed that it requires calling some external system API, I am not sure will it become a problem of cross-compilation or not?@marioortizmanero commented on GitHub (Aug 13, 2020):
Looks great! Here's what I came up with:
Iterators are cool! :)
getrandomjust uses random number generators from the operating system rather than implementing them from "scratch", likerand. I don't think it's a problem, as it supports all operating systems I know of.@marioortizmanero commented on GitHub (Aug 13, 2020):
Here's a comparison I've made:
randversion:getrandomversion:du -hafter runningstrip, with --releaseThe
cargo treeoutput from therandversion actually includesgetrandom, so I suspect the speed is similar just becauserandin the end usesgetrandom. The change togetrandomseems worth it to me.@ramsayleung commented on GitHub (Aug 14, 2020):
Cool, I just open a PR https://github.com/ramsayleung/rspotify/pull/110 to reduce core dependencies, feel free to contribute to this.
Just out of curiosity, would you like to add
oorandomto this contest, I am just curious about how does it perform, is it 10x lightweight thanrandas it claims inREADME.@marioortizmanero commented on GitHub (Aug 14, 2020):
Sure, I've added a benchmark with
oorandomjust out of curiosity, since it doesn't make sense to use it for this crate.@marioortizmanero commented on GitHub (Aug 28, 2020):
This can be closed now that #110 was merged.