Make ChannelTask public, but hidden in docu#7
Conversation
There was a problem hiding this comment.
Pull request overview
Exposes ChannelTask construction for niche advanced use cases while adding feature-gated serializer backends so the crate can use either postcard (default) or pot for message encoding/decoding.
Changes:
- Add mutually exclusive
codec-postcard/codec-potfeature flags and implementto_bytes/from_bytesfor both codecs. - Make
ChannelTask::newpublic but hidden from generated docs. - Update dependencies and CI clippy job to validate both codec feature sets.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/convert.rs |
Adds postcard/pot codec implementations behind mutually exclusive feature flags and enforces selection via compile_error!. |
src/channel_task.rs |
Makes ChannelTask::new public (but #[doc(hidden)]) so users can construct tasks manually. |
Cargo.toml |
Makes postcard optional, adds optional pot, introduces codec features, and updates default features. |
Cargo.lock |
Records new dependency resolution including pot and its transitive deps. |
.github/workflows/test.yml |
Replaces --all-features clippy with two clippy runs (one per codec). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
paberr
left a comment
There was a problem hiding this comment.
Thanks for the PR!
One note: in it's current form, the codec change is breaking for users with default-features = false.
postcard was previously unconditional, and now they'd hit a compile_error!.
I wonder whether a better design would be to have postcard still enabled by default, and a pot feature that replaces it with pot?
The only downside I can think of is that postcard stays as a dependency even with codec-pot, that's a few extra KB.
|
I made the changes as requested by you. Only pot feature is totally fine for me. But it would mean with -all-features this is then the used codec. From my perspective perfect, because it's the codec I use. I just have seen you released v0.3.0 today. Too bad, I'm too late for that.... |
Oh true, I didn't think about that. Let me think about what's the best method then.
Not a big issue, I'm happy to release new versions quickly. 😄 |
|
I thought about this more and think the following pattern might be the cleanest: Make both codecs optional, with postcard taking priority when both are enabled: [features]
default = ["serde", "codec-postcard"]
codec-postcard = ["dep:postcard"]
codec-pot = ["dep:pot"]convert.rs #[cfg(feature = "codec-postcard")]
pub fn to_bytes<T: Serialize>(value: &T) -> Box<[u8]> {
postcard::to_allocvec(value).expect("WebWorker serialization failed").into()
}
#[cfg(all(feature = "codec-pot", not(feature = "codec-postcard")))]
pub fn to_bytes<T: Serialize>(value: &T) -> Box<[u8]> {
POT_CONFIG.serialize(value).expect("WebWorker serialization failed").into()
}
#[cfg(not(any(feature = "codec-postcard", feature = "codec-pot")))]
compile_error!("No codec selected. Enable `codec-postcard` (default) or `codec-pot`.");That combination gives us:
The Also, can you rebase? After that change, I'll let CoPilot review it once more. If everything is fine, I'll merge and release. |
|
Done, have a check and let me know :) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Thanks a lot @Roba1993 ! |
|
It's part of the |
So now I'm a bit late with a response :sweat_smile
Thank you very much for your rework and the release of the new version.
My problem with postcard were SerializeSeqLengthUnknown errors, which I have fixed right now.
Right now I need to have the ChannelTask creation available.
The reason is, sometimes I want to execute certain logic in the main thread or WebWorker. In this cases my custom logic needs the channel in the same way as when I work with WebWorker.
I understand, that this is quiet niche, so I hid the new function from the docs.