chore: add jiff behind a feature flag #39
Conversation
davidbarsky
changed the title
ipetkov
left a comment
ipetkov
left a comment
There was a problem hiding this comment.
Overall LGTM, thanks for working on this!
Mostly have some questions around scalar names (and whether we can make the actual Rust time backend opaque to the schema consumers)
Feel free to flip this out of draft and get the CI to run whenever you're ready
| #[cfg(all(feature = "jiff-0_2", not(feature = "chrono")))] | ||
| use jiff::Timestamp; | ||
| #[cfg(feature = "chrono")] | ||
| use chrono::{DateTime, Utc}; |
There was a problem hiding this comment.
I'm okay with the apollo tracing extension requiring jiff and not supporting chrono here (should be easier without having to deal with the cfg soup)
| [features] | ||
| apollo_persisted_queries = ["lru", "sha2"] | ||
| apollo_tracing = ["chrono"] | ||
| apollo_tracing = [] |
There was a problem hiding this comment.
See my other comment, I'm fine with this requiring dep:jiff
| #[Scalar( | ||
| internal, | ||
| name = "Duration", | ||
| name = "ChronoDuration", |
There was a problem hiding this comment.
Can we keep this scalar name as Duration or will it collide with jiff's version?
The reason I ask is because if these are meant to be ISO 8601 durations, then the backing implementation (whether chrono or jiff) shouldn't actually matter to a GraphQL consumer. Also swapping one backend for another shouldn't require churning the schemas (assuming they are being either committed somewhere or the consumers are using something like Apollo Client for code generation)
| name = "ChronoDateTimeFixedOffset", | ||
| specified_by_url = "https://datatracker.ietf.org/doc/html/rfc3339" | ||
| )] | ||
| impl ScalarType for DateTime<FixedOffset> { | ||
| fn parse(value: Value) -> InputValueResult<Self> { | ||
| match &value { | ||
| Value::String(s) => Ok(s.parse::<DateTime<FixedOffset>>()?), | ||
| _ => Err(InputValueError::expected_type(value)), | ||
| } | ||
| } | ||
|
|
||
| fn to_value(&self) -> Value { | ||
| Value::String(self.to_rfc3339()) | ||
| } | ||
| } | ||
|
|
||
| /// Implement the DateTime<Local> scalar | ||
| /// | ||
| /// The input/output is a string in RFC3339 format. | ||
| #[Scalar( | ||
| internal, | ||
| name = "DateTime", | ||
| name = "ChronoDateTimeLocal", | ||
| specified_by_url = "https://datatracker.ietf.org/doc/html/rfc3339" | ||
| )] | ||
| impl ScalarType for DateTime<Local> { | ||
| fn parse(value: Value) -> InputValueResult<Self> { | ||
| match &value { | ||
| Value::String(s) => Ok(s.parse::<DateTime<Local>>()?), | ||
| _ => Err(InputValueError::expected_type(value)), | ||
| } | ||
| } | ||
|
|
||
| fn to_value(&self) -> Value { | ||
| Value::String(self.to_rfc3339()) | ||
| } | ||
| } | ||
|
|
||
| /// Implement the DateTime<Utc> scalar | ||
| /// | ||
| /// The input/output is a string in RFC3339 format. | ||
| #[Scalar( | ||
| internal, | ||
| name = "DateTime", | ||
| name = "ChronoDateTime", |
There was a problem hiding this comment.
Same comment on keeping the names the same as before
| #[Scalar( | ||
| internal, | ||
| name = "TimeZone", | ||
| name = "ChronoTz", |
|
|
||
| use crate::{InputValueError, InputValueResult, Scalar, ScalarType, Value}; | ||
|
|
||
| #[Scalar(internal, name = "JiffDate")] |
There was a problem hiding this comment.
Same comment on these scalar names, is it possible to use the same scalar name as chrono uses to avoid churning the actual schema?
2 participants
Bit of a draft, but adding jiff for the same reasons as outlined in jj-vcs/jj#8038.