Fix raw response with selections on abstract type #4775
Conversation
tobias-tengler
force-pushed
the
fix-raw-response-with-selections-on-abstract-type
branch
from
b883382 to
32f5717
Compare
|
@captbaritone Thoughts on this? |
|
This looks more correct to me at first glance! Thanks for working on this. I likely won't have time to look into it more closely until after GraphQL conf. One thing we'll have to look out for is if this will cause type errors with existing places people are using raw response types and how we'll manage that rollout if it's the case. Let me see if anyone else on the team can take a look before then. |
tobias-tengler
force-pushed
the
fix-raw-response-with-selections-on-abstract-type
branch
2 times, most recently
from
b2a047c to
97221cb
Compare
|
@captbaritone This will certainly lead to type errors for existing projects. I've added a feature flag to enable an opt-out. |
tobias-tengler
force-pushed
the
fix-raw-response-with-selections-on-abstract-type
branch
from
97221cb to
45b2e04
Compare
tobias-tengler
force-pushed
the
fix-raw-response-with-selections-on-abstract-type
branch
from
45b2e04 to
2be6f8c
Compare
tobias-tengler
force-pushed
the
fix-raw-response-with-selections-on-abstract-type
branch
from
2be6f8c to
af3c1c4
Compare
tobias-tengler
force-pushed
the
fix-raw-response-with-selections-on-abstract-type
branch
from
af3c1c4 to
a5db816
Compare
|
@captbaritone Could we land this? |
| @@ -120,6 +142,7 @@ pub(crate) struct RawResponseFragmentSpread { | |||
| pub(crate) value: StringKey, | |||
| pub(crate) conditional: bool, | |||
| pub(crate) concrete_type: Option<Type>, | |||
| pub(crate) abstract_type: Option<Type>, | |||
There was a problem hiding this comment.
Are there actually cases where a selection (like this one or the ones below) actually have both a concrete type and an abstract type? My reading of this PR is that we expect to have only one or the other or neither. Is my reading correct? If it is, could we collapse this to an option of an enum with two variants?
| .get_object_id() | ||
| .map(|id| typegen_context.schema.object(id)) | ||
| { | ||
| for (abstract_type, abstract_selections) in &by_abstract_type { |
There was a problem hiding this comment.
Could we get a comment explaining the case we are handling here?
| if !base_fields.is_empty() || !by_abstract_type.is_empty() { | ||
| let mut base_fields_map = selections_to_map(base_fields.clone().into_iter(), false); | ||
|
|
||
| if !by_abstract_type.is_empty() { |
There was a problem hiding this comment.
Could we get a comment explaining the case we are handling here?
|
@captbaritone Thanks for the review! |
I suspect we will. |
Previously all selections done on abstract types, would be generated as required properties in the raw response type.
This PR tracks all selections on abstract types separately and only makes them required if the concrete type implements the abstract type. The change in the generated types can be seen in this diff.
To enable a gradual rollout, a
disable_more_precise_abstract_selection_raw_response_typefeature flag has been added that allows to opt operations out of the new raw response type generation.