Move data streams into their own crate#1227
Conversation
Not needed for now, can be re-added with an feature later
ChangesetThe following package versions will be affected by this PR:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1egoman
force-pushed
the
data-streams-refactor
branch
from
1e4e420 to
4492d6e
Compare
|
|
||
| [features] | ||
| # End-to-end testing hooks (exposes is_compressed/is_inline on stream info). Forwarded from | ||
| # the `livekit` crate's `__lk-e2e-test` feature. | ||
| __e2e-test = [] | ||
| # Exposes constructors used by downstream crates' test suites (e.g. `TextStreamReader::new_for_test`). | ||
| test-utils = [] |
There was a problem hiding this comment.
Note to reviewers: should these be separate features or just one?
There was a problem hiding this comment.
If we need to keep them, they should be separate (I plan to eventually drop the special feature for E2E testing and instead just skip E2E tests based on env).
However, there's the bigger question of if we should be using non-public methods in integration tests in the first place.
There was a problem hiding this comment.
However, there's the bigger question of if we should be using non-public methods in integration tests in the first place.
Right now, that's how it works so I don't think this makes it worse. IMO I'd rather decouple that conversation and revisit as part of the data streams v2 pull request since I will be making some more extensive updates there.
There was a problem hiding this comment.
If we need to keep them, they should be separate (I plan to eventually drop the special feature for E2E testing and instead just skip E2E tests based on env).
I've combined them into one feature for now, test-utils.
…d CLIENT_PROTOCOL_DATA_STREAM_V2 from livekit-common
|
suggestion: To prevent future abuse of the livekit-common crate, it might be a good idea to describe it in AGENTS.md (or add a new one scoped to the crate) laying out what belongs here and what doesn't (e.g., lightweight functions, types, etc.). |
| mgr.handle_chunk(chunk("s1", 0, text.as_bytes().to_vec()), EncType::None); | ||
| mgr.handle_trailer(trailer("s1")); | ||
| assert_eq!(read_text(reader).await.unwrap(), text); | ||
| } |
There was a problem hiding this comment.
Notable improvement: I've added in an initial set of data streams v1 unit tests which exercises some happy paths in the data streams outgoing manager and data streams incoming manager. These will be further developed as part of the data streams v2 pull request.
Co-authored-by: Jacob Gelman <3182119+ladvoc@users.noreply.github.com>
Done in 4445b08 |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| thiserror = { workspace = true } | ||
| parking_lot = { workspace = true } | ||
| bytes = { workspace = true } | ||
| tokio = { workspace = true, default-features = false, features = ["sync", "fs", "io-util", "rt"] } |
There was a problem hiding this comment.
note: Outside the scope of this PR and a pre-existing issue, but we should only depend on tokio with the sync feature unconditionally.
There was a problem hiding this comment.
Good to know, I will investigate in the data-streams-v2 follow up pull request.
3 participants
Breaks out a subset of the changes from #1192 into a pull request which exclusively moves the existing data streams implementation into its own crate,
livekit-data-streams, and adds a newlivekit-commoncrate (kept fairly minimal for now) for common utilities whichlivekit-data-streamsandlivekitboth need.This is largely being scoped to a "lift and shift" type effort, more extensive refactoring of the
livekit-data-streamscrate will be done in #1192.Breaking changes
There should not be any breaking changes from the perspective of
livekit's public api. If so, please surface them.Testing
This pull request adds the relevant v1 data streams tests from #1192. More test coverage will be introduced as a follow up in the data streams v2 change.