add magic link field
#282
add magic link field
#282
Conversation
|
should we name the field |
|
And do we fail if there's a link already, or do we ignore? |
And this is an issue, because |
|
I think we should panic with a helpful message. Eventually we can allow the user to provide a directive in a schema extension to rename the |
|
And maybe once we have namespaces, we can have an "internal" namespace, and the user can selected No more conflicts! Idk |
| @@ -196,28 +196,53 @@ pub(crate) fn generate_eager_reader_param_type_artifact( | |||
|
|
|||
| let mut param_type_imports = BTreeSet::new(); | |||
| let mut loadable_fields = BTreeSet::new(); | |||
| let mut link_fields = BTreeSet::new(); | |||
crates/graphql_artifact_generation/src/eager_reader_artifact.rs
Outdated
Show resolved
Hide resolved
| @@ -199,6 +199,7 @@ pub struct UserWrittenClientFieldInfo { | |||
| pub enum ClientFieldVariant { | |||
| UserWritten(UserWrittenClientFieldInfo), | |||
| ImperativelyLoadedField(ImperativelyLoadedFieldVariant), | |||
| Link, | |||
There was a problem hiding this comment.
Thinking out loud... yes, a link field is a client field. The id aspect of it is magical, though, so yes, it sounds like it should be a separate ClientFieldVariant. Okay. Sgtm.
| @@ -56,7 +56,31 @@ impl UnvalidatedSchema { | |||
| Span::todo_generated(), | |||
| ); | |||
|
|
|||
| let condition_selection_set = vec![typename_selection]; | |||
| let link_selection = WithSpan::new( | |||
There was a problem hiding this comment.
note to self got to here
| location: FieldType::ClientField( | ||
| match *subtype | ||
| .encountered_fields | ||
| .get(&"link".intern().into()) |
There was a problem hiding this comment.
nit: can "link".intern().into() be defined in a lazy_static block and we use that everywhere?
| match *subtype | ||
| .encountered_fields | ||
| .get(&"link".intern().into()) | ||
| .expect("Expected link to exist") |
There was a problem hiding this comment.
I don't think it's ideal that .encountered_fields is just a hash map. IMO it would be better to have a struct with special slots for all the special fields: __typename, an optional id, link, etc. and a hash map for all of the regular fields. that way, we don't have to do more assertions here.
| &scalar_field_selection.arguments, | ||
| ), | ||
| None => match newly_encountered_scalar_client_field.variant { | ||
| ClientFieldVariant::Link => {} |
There was a problem hiding this comment.
should we merge the link field's selection set here? I can't actually get it to exhibit behavior in which the typename is not selected but should be, so I think that would be unnecessary. But can we rely on that behavior?
Anyway, I don't know the right answer here, happy to go with whatever you think is best
There was a problem hiding this comment.
I think we don't need to merge here, we are guaranteed to have __typename since cache uses two layers now
This is a bit awkward at places because
Loadability, should we do that?__link@loadably, we should validate it as part of this PRtype Linkis not imported right know, but it's not an issue since client pointers are not implemented yetother then that It seems to work