FED-189: port extractDeferFromOperation #284
Conversation
duckki
requested review from
dariuszkuc,
sachindshinde,
goto-bus-stop and
SimonSapin
as code owners
- `register_defer` call was previously missed. - removed DeferDirectiveArgs, in favor of DeferDirectiveArguments - removed FetchDependencyGraphPath, in favor of FetchDependencyGraphNodePath
|
About the |
src/link/graphql_definition.rs
Outdated
| pub(crate) label: Option<NodeStr>, | ||
| pub(crate) if_: Option<BooleanOrVariable>, |
There was a problem hiding this comment.
Not sure if there is a reason for these being private. It seems like there were just two spots that make use of this change. One of those places can be replaced with an existing method. The other can be simplified to not need to use a struct literal.
| pub(crate) label: Option<NodeStr>, | |
| pub(crate) if_: Option<BooleanOrVariable>, | |
| label: Option<NodeStr>, | |
| if_: Option<BooleanOrVariable>, |
src/query_graph/graph_path.rs
Outdated
| // Note: @defer is not repeatable. | ||
| inline_fragment.data().directives.get("defer").map(|defer| { | ||
| let label: Option<NodeStr> = defer | ||
| .argument_by_name("label") | ||
| .and_then(|label| label.as_node_str().cloned()); | ||
| let if_ = directive_optional_variable_boolean_argument(defer, &name!("if")) | ||
| .unwrap_or_default(); | ||
| DeferDirectiveArguments { label, if_ } | ||
| }) |
There was a problem hiding this comment.
There is already a method for this on NormalizedInlineFragmentData, so you can replace all of this with:
| // Note: @defer is not repeatable. | |
| inline_fragment.data().directives.get("defer").map(|defer| { | |
| let label: Option<NodeStr> = defer | |
| .argument_by_name("label") | |
| .and_then(|label| label.as_node_str().cloned()); | |
| let if_ = directive_optional_variable_boolean_argument(defer, &name!("if")) | |
| .unwrap_or_default(); | |
| DeferDirectiveArguments { label, if_ } | |
| }) | |
| inline_fragment.data().defer_directive_arguments().ok().flatten() |
| pub(crate) path_in_node: Arc<OpPath>, | ||
| pub(crate) response_path: Vec<FetchDataPathElement>, |
There was a problem hiding this comment.
Let's limit visibility for fields that don't need to be made public.
| pub(crate) path_in_node: Arc<OpPath>, | |
| pub(crate) response_path: Vec<FetchDataPathElement>, | |
| path_in_node: Arc<OpPath>, | |
| response_path: Vec<FetchDataPathElement>, |
| return Ok((Some(operation.clone()), updated_context)); | ||
| }; | ||
|
|
||
| let Some(ref updated_defer_ref) = defer_args.label else { |
There was a problem hiding this comment.
Use the existing label method:
| let Some(ref updated_defer_ref) = defer_args.label else { | |
| let Some(updated_defer_ref) = defer_args.label() else { |
| return Err(FederationError::internal( | ||
| "All defers should have a label at this point", | ||
| )); |
There was a problem hiding this comment.
Also, this is more stylistic, but using Option::ok_or_else + a try operator (i.e. ?) is often an easier-to-read way of short-circuiting a function that returns a Result when you have an Option. Not a necessary change, as both ways will compile to basically the same thing, but I figured I would mention it.
|
@TylerBloom Thanks for the review! I just accepted your suggestions and pushed it. |
…deration-next#284) - ported `extractDeferFromOperation`
Implemented
extract_defer_from_operationfunction.Even though most of its functionality won't be used until
@deferis actually implemented, it's called fromcompute_nodes_for_op_path_elementand we would've needed some implementation anyways. So, I ported all of it.