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

make it possible to disable cookies and optimize startup #578

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jeremyandrews
Copy link
Member

@jeremyandrews jeremyandrews commented Dec 22, 2023

  • add new cookies feature, enabled by default
  • optimize GooseUser startup if cookies are disabled
  • only compile Reqwest Cookies if cookies feature is enabled in Goose
  • document
  • fixes Speed up user initialization #557

By making this a compile-time feature, we can enforce at compile-time that nobody expects cookies to work properly when using this startup optimization. Attempts to use Reqwest's cookie-related functions will fail if the Goose cookie feature is disabled. This also removes the cookie dependencies when it's not needed.

@jeremyandrews jeremyandrews changed the title make it possible to disable cookies make it possible to disable cookies and optimize startup Dec 22, 2023

```toml
[dependencies]
goose = { version = "^0.17", default-features = false, features = ["reqwest/default-tls"] }
Copy link
Contributor

@Michael-Hollister Michael-Hollister Dec 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When copying the example for testing the branch, I got this error below. Perhaps there should be additional cargo features that only enable default-tls/rustls-tls without including cookies?

error: failed to parse manifest at `Cargo.toml`

Caused by:
  feature `reqwest/default-tls` in dependency `goose` is not allowed to contain slashes
  If you want to enable features of a transitive dependency, the direct dependency needs to re-export those features from the `[features]` table.

Copy link
Collaborator

@LionsAd LionsAd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally find the many cfg things to add a lot of complexity.

I am okay with some very restricted changes, but this PR feels overkill to me.

As far as I understand the slowdown is mostly from reqwest cookie feature itself, so removing that is fine, but cluttering the code with cfg feels like premature optimization.

@jeremyandrews
Copy link
Member Author

As far as I understand the slowdown is mostly from reqwest cookie feature itself, so removing that is fine, but cluttering the code with cfg feels like premature optimization.

Yes, the added config option is not an attempt to speed up or optimize anything. It's so the compiler can catch if someone 1) disables cookies, and then 2) tries to use cookie functinality: it will fail to compile. Without the config option, a person could disable cookies but use the code causing a panic or other undefined errors.

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

Successfully merging this pull request may close these issues.

3 participants