refactor(core): move AckStream, DropStream, ResponseHandlers and ChanStream into core behind remote-adapter feature#758
Conversation
…Stream into core behind remote-adapter feature Moves the duplicated stream constructs from all three remote adapters (redis, postgres, mongodb) into socketioxide-core behind the existing remote-adapter feature flag. Removes 737 lines of duplicated code. - DropStream<S, T>: generic cleanup-on-drop stream wrapper - AckStream<S, R, T, E>: generic ack stream merging local + remote - ResponseHandlers<T>: generic type alias for HashMap<Sid, mpsc::Sender<T>> - ChanStream<T>: generic mpsc::Receiver stream wrapper Each adapter now provides a decode function passed as a fn pointer to AckStream, avoiding orphan rule issues with trait impls on foreign types like Vec<u8>. Closes Totodore#756
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Totodore
added
A-core
|
@Totodore this is ready for review. The 3 clippy code-scanning comments are now fixed (module docs + AckDecoder type alias). The cargo test --all-features --workspace CI failure appears pre-existing on main, looks related to fred/redis-cluster optional feature doctests. |
|
CI is green on main, https://github.com/Totodore/socketioxide/actions/runs/29045217076. So there is no reason for it to be red here. Formatting is not even ok. |
|
engine_io (V3) seems to be a flaky e2e test, this changes dont touch anything in engineioxide. could you re-run the job? |
Totodore
left a comment
Totodore
left a comment
There was a problem hiding this comment.
Neat!
Just some comments, not exhaustive though as I'm on my phone.
I will do a more thorough review later.
Co-authored-by: Théodore Prévot <prevottheodore@gmail.com>
… 11 AckStream tests
|
Okay, looks like all green man, except socket_io (v4), which i expect to be a flaky e2e test again, try to re run as soon as you can, if not i will do a further analysis to see if maybe it is an actual regression. |
Totodore
left a comment
Totodore
left a comment
There was a problem hiding this comment.
Please also check dependencies feature flags!
…place new_empty_remote with Either, move decode fns to lib.rs - AckStream<E: SocketEmitter, R: Stream, T> reduces 4 params to 3 - AckDecoder<I, E> swapped param order (item first, error second) - new_empty_remote removed; adapters use Either<E::AckStream, AckStream<...>> - decode fns moved from stream.rs to lib.rs in redis/mongodb - Duplicate adapter tests removed (covered in core) - Redis/MongoDB stream.rs deleted; Postgres stream.rs trimmed to AckPayload only
|
These stale alerts are from earlier commits, all fixed in the latest push. cargo clippy --all-features --workspace returns 0 errors. |
|
I found out what i was missing, sorry man, all review comments addressed. |
|
Some clippy warning are still here. Could you please check them. Otherwise we're good to go. In the meantime I'm going to try to fix the ci that doesn't want to report those warnings. |
3 participants
Motivation
All three remote adapters (redis, postgres, mongodb) independently define AckStream, DropStream, and ResponseHandlers with near-identical code. This duplication makes maintenance harder and is contrary to the goal of #727 (share more code between adapters).
Solution
Move AckStream<S, R, T, E>, DropStream<S, T>, ResponseHandlers, and ChanStream into socketioxide-core behind the existing remote-adapter feature flag. Each adapter now provides a stateless decode function passed as a fn pointer to AckStream::new(), avoiding orphan rule issues that a trait-based approach would cause when implementing on foreign types like Vec.