Skip to content

Preserve the order of requested fields #85

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

Merged
merged 1 commit into from
Sep 2, 2017

Conversation

LegNeato
Copy link
Member

Fixes #82

Note: I am new to Rust, please let me know if there is anything I can do better.

@theduke
Copy link
Member

theduke commented Aug 28, 2017

Since the spec recommends it, I'm definitely in favor.

Thanks for the PR, @LegNeato!

There are two open questions for me:

Is there maybe another order preserving hashmap crate around which is better maintained?

@LegNeato
Copy link
Member Author

Re: perf, I couldn't get cargo bench to work locally...any pointers would be appreciated.

Re: linked-hash-map, I was hesitant as well. https://github.com/bluss/ordermap looks better maintained but doesn't yet have serde support (indexmap-rs/indexmap#28) which juniper needs. Also, linked-hash-map maintainers said they will merge changes to keep it compatible and building...which I think is all we really need for juniper in the future. I intend to keep an eye on ordermap and switch if/when it has serde support.

@rushmorem
Copy link
Contributor

Can't we switch to OrderMap and use remote derive for now?

@LegNeato
Copy link
Member Author

I just tried and it doesn't look like you can use remote derive for this, as just using

#[derive(Serialize, Deserialize)]
#[serde(remote = "OrderMap")]

Fails as the default derive isn't sufficent and

#[serde(remote = "OrderMap")]

fails without derive().

I'm a Rust n00b, so if there is a way to do this that I am missing please let me know!

@LegNeato
Copy link
Member Author

I got them to do a release, so latest changes use ordermap.

@bluss
Copy link

bluss commented Aug 30, 2017

Nice to see that you get some utility out of ordermap. As a point of order, you'll should probably add Ordermap's 1.0-ing on your own 1.0 road map, since it's a “public dependency” when used like this.

OrderMap itself doesn't have an 1.0 road map, for the record, but the future is open ended.

@mhallin
Copy link
Member

mhallin commented Sep 2, 2017

Thanks for this PR! Bringing us closer to spec compliance :)

@mhallin mhallin merged commit 5d43532 into graphql-rust:master Sep 2, 2017
@LegNeato LegNeato deleted the request_order branch August 25, 2023 21:24
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.

5 participants