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

dropshot has implicit dependency on schemars and serde versions #67

Open
davepacheco opened this issue Dec 22, 2020 · 5 comments
Open

Comments

@davepacheco
Copy link
Collaborator

Dropshot expects some of the consumer's types to impl schemars::JsonSchema, serde::Serialize, and serde::Deserialize. Today, I think consumers add schemars and serde to their Cargo.toml with the same versions as Dropshot and this works fine. However, it's not obvious to consumers that you'd need to do this, or what version(s) would be acceptable here.

I'm trying to better understand the best practice in Rust around this. I expect we'll either want to export JsonSchema from Dropshot and have consumers use that (so they're always using a compatible version, and it's clear where it comes from and what version it is) or else document the dependencies. I'm not sure about serde's interfaces. It seems like overkill to export these, but it also seems like the more correct approach here.

This came up because JsonSchema updated to 0.8. While we updated Dropshot in the master branch (see #61), we didn't bother publishing this to crates.io. So if you grab that version and try to use it with the latest JsonSchema, you get an error about your type not implementing JsonSchema.

@ahl
Copy link
Collaborator

ahl commented Dec 22, 2020

For JsonSchema I think we should both export it and maybe wrap up the derive with a macro of our own. I'm 90% we'll want to swap out schemars at some point so better to use our own macro that uses it internally.

Not sure what we should do with regard to serde but happy to have this issue assigned to me either way.

@ahl
Copy link
Collaborator

ahl commented Dec 24, 2020

It looks like it may not be possible to re-export serde::{Deserialize,Serialize}: serde-rs/serde#1465

@ahl
Copy link
Collaborator

ahl commented Jan 6, 2021

This turns out to be unavoidable for the reasons cited in #70. As @davepacheco suggested in the description: let's just document these dependencies and versions.

@jtroo
Copy link

jtroo commented Jun 14, 2021

There is a workaround that's found in this comment and is also made use of in the Rocket examples.

The rocket example:

#[derive(Debug, Clone, Deserialize, Serialize)]
#[serde(crate = "rocket::serde")]
struct Post {
    ...

@ahl
Copy link
Collaborator

ahl commented Jun 16, 2021

@jtroo thanks for pointing that out. It looks a little onerous and error-prone for consumers to need to specify that extra line for each serde derive.

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 a pull request may close this issue.

3 participants