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

Rfc30/http #2198

Conversation

thomas-k-cameron
Copy link
Contributor

Motivation and Context

Part of RFC30.
Implements relevant logic for http/eventstream crate.

Description

  • add serde on external-type
  • add serde-serialize/serde-deserialize

Testing

Testing is not implemented yet.

Checklist

CHANGELOG is updated once all the PR is approved.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@thomas-k-cameron
Copy link
Contributor Author

@rcoh
#1944 (comment)

About this one, how do you think that it should handled?

@thomas-k-cameron thomas-k-cameron changed the base branch from main to unstable-serde-support January 13, 2023 01:02
Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

At a higher level, I think it would be better to not support ser/de at all for operations that feature event streams since the stream is ephemeral (tied to the HTTP connection), and is effectively unusable after serialization and deserialization. Most event stream operations also don't have fields that go along with them, making the stream the sole component in them, which makes ser/de not so useful.

Rather than this approach, I think we should adjust the code generator to not add the ser/de implementations to any type that has an event stream type in it.

@rcoh
Copy link
Collaborator

rcoh commented Jan 19, 2023

yeah I agree with @jdisanti — it's a two-way-door to add ser/de in the future. I think it's best to hold off on this and consider adding it at the end

@thomas-k-cameron
Copy link
Contributor Author

I don't really have a strong opinion about event streams to be honest.

I looked up the crates.io but sdk that uses event-stream got less than 3000 all-time downloads, which makes it difficult to justify implementing something that is potentially harmful.

#2198 (comment)
You could create a new struct which copies it's twin without the field for the event stream, if there is a demand.

e.g.

struct OriginalStruct {
   event_stream: EventStreamType,
   other_field: u64
}

#[derive(serde::Serialize)]
struct Twin {
   other_field: u64
}

@thomas-k-cameron
Copy link
Contributor Author

Let me update the RFC30 to make sure we are on the same pages.

@thomas-k-cameron
Copy link
Contributor Author

@Velfi
@jdisanti
@rcoh

Can you guys check it out?
#2243

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.

4 participants