Skip to content

refactor: tls backend feature flags#89

Open
dman-os wants to merge 1 commit intoalexjg:mainfrom
dman-os:refactor/ssl-backend-flags
Open

refactor: tls backend feature flags#89
dman-os wants to merge 1 commit intoalexjg:mainfrom
dman-os:refactor/ssl-backend-flags

Conversation

@dman-os
Copy link
Copy Markdown

@dman-os dman-os commented Mar 17, 2026

Feature flags to configure TLS impl for tokio ws.

Disclaimer

This PR was entirely written by a Codex agent.

@dman-os dman-os changed the title refactor: ssl backend feature flags refactor: tls backend feature flags Mar 17, 2026
@alexjg
Copy link
Copy Markdown
Owner

alexjg commented Mar 25, 2026

Rather than doing this awkward wrangling with feature flags I think it would be better to implement alternative TLS strategies in another crate as a custom Transport (and Dialer and Acceptor) implementation. The dial_websocket method is very much just a convenience function for the common case.

@dman-os
Copy link
Copy Markdown
Author

dman-os commented Mar 25, 2026

@alexjg do you mean a crate external to this repo? Also, if you intend to keep dial_websocket in samod proper, I think these standard feature flags would be preferable to having a separate crate duplicating the tungstenite support code just to change the TLS.

@alexjg
Copy link
Copy Markdown
Owner

alexjg commented Mar 25, 2026

Yeah maybe we should just have another crate for all websocket related things. The reason I am averse to the feature flags is that they don't compose nicely, if I'm reading the docs on the packages in question correctly then you have to only enable one of them and features are additive, so it's easy for both features to get switched on by different parts of the dependency graph and now you have a weird compilation error that's hard to track down because you've gone through three crates worth of feature flags (tungstenite, tokio-tungstenite, samod). Maybe I'm misunderstanding the documentation though and it's fine to enable all of them?

@dman-os
Copy link
Copy Markdown
Author

dman-os commented Mar 25, 2026

I assumed since we're delegating to the tungstenite features here, that crate would have it's own incompatibility blocking. Let me double check! And yeah, if you don't add the tls feature flags, you don't get any TLS.

@dman-os
Copy link
Copy Markdown
Author

dman-os commented Mar 25, 2026

Looks like you can enable both flags for tokio-tungestenite but native-tls is given preferrence: https://github.com/snapview/tokio-tungstenite/blob/57fc3d0276564efaf11efe8c1499b26b44dbe9a4/src/tls.rs#L221-L232

I do understand the confusion concern. I just deferred to established ecosystem conventions here as seen in big crates like https://lib.rs/sqlx.

@alexjg
Copy link
Copy Markdown
Owner

alexjg commented Mar 25, 2026

Okay great (and thanks for elaborating) in that case if you could rebase the PR and update the commit to move the note in CHANGELOG.md to be under a new section called ## Unreleased then I can merge.

@dman-os dman-os force-pushed the refactor/ssl-backend-flags branch from a81e9a7 to 41cd8d5 Compare March 25, 2026 22:20
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.

2 participants