From 3a151b45acb9e3b582ce82c7002bcb252081cbb7 Mon Sep 17 00:00:00 2001 From: Andrey Lunyov Date: Fri, 21 Oct 2022 16:28:21 -0700 Subject: [PATCH] =?UTF-8?q?Terser=20Syntax=20+=20Relay=20Models=20=3D=20?= =?UTF-8?q?=E2=9D=A4=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: This diff connects the enables Terse Syntax with the new Relay Resolver Models. I want to have another pass over `relay-docblock` parser after this diff. Currently, we're passing `schema` to some of the methods in the `RelolverIr` trait, we can probably avoid this, and make it a little cleaner. But for now, to make it work, let's keep it simple. Specifically, in this diff - OutputType of the terser resolver is `Pending` and we are processing this pending type during the schema extension generation (where we have access to `&SDLSchema` - Fixed directives for terser syntax (these should be defined on the field, not on the type). - Implementation for `root_fragment` is shared between Resolver and TerserResolver IR --- ## Comparison of the old and the new API: Before: ``` type MyClientType { id: ID! } /** * RelayResolver * onType MyClientType * fieldName self * rootFragment MyClientType_selfFragment * live */ function MyClientType_self(rootKey: MyClientType_selfFragment$key): MyClientTypeFlow { const data = readFragment(graphql` fragment MyClientType_selfFragment on MyClientType { id }`, rootKey, ); return externalState.readAndSubscribe(data.id); } ``` After: ``` /** * RelayResolver MyClientType * live */ function MyClientType(id: DataID): MyClientTypeFlow { return externalState.readAndSubscribe(string); } ``` ### Individual Resolvers Before: ``` /** * RelayResolver * onType MyClientType * fieldName description */ function MyClientType_description(rootKey: MyClientType_descriptionFragment$key): string { const data = readFragment(graphql` fragment MyClientType_descriptionFragment on MyClientType { self }`, rootKey, ); return data?.self?.description } ``` After: ``` /** * RelayResolver MyClientType.description: String */ function MyClientType(instance: ?MyClientTypeFlow): ?string{ return instance?.description; } ``` Reviewed By: captbaritone Differential Revision: D40541088 fbshipit-source-id: 0ad921f845c7360738fa909430c3acacb7fa865d --- compiler/crates/relay-docblock/src/ir.rs | 149 +++++++++++------- compiler/crates/relay-docblock/src/lib.rs | 8 +- .../fixtures/terse-relay-resolver.expected | 1 - ...e-relay-resolver-with-output-type.expected | 4 +- .../fixtures/terse-relay-resolver.expected | 4 +- .../__tests__/resolvers/QueryTodoModel.js | 5 +- .../__tests__/resolvers/TodoDescription.js | 10 +- .../store/__tests__/resolvers/TodoModel.js | 9 +- scripts/config.tests.json | 1 + 9 files changed, 108 insertions(+), 83 deletions(-) diff --git a/compiler/crates/relay-docblock/src/ir.rs b/compiler/crates/relay-docblock/src/ir.rs index 3d49239e67954..3c20a29da4035 100644 --- a/compiler/crates/relay-docblock/src/ir.rs +++ b/compiler/crates/relay-docblock/src/ir.rs @@ -150,8 +150,9 @@ impl Named for Argument { } } -#[derive(Debug, PartialEq)] +#[derive(Debug, PartialEq, Clone)] pub enum OutputType { + Pending(WithLocation), EdgeTo(WithLocation), Output(WithLocation), } @@ -159,6 +160,7 @@ pub enum OutputType { impl OutputType { pub fn inner(&self) -> &WithLocation { match self { + Self::Pending(inner) => inner, Self::EdgeTo(inner) => inner, Self::Output(inner) => inner, } @@ -183,7 +185,7 @@ trait ResolverIr { fn definitions(&self, schema: &SDLSchema) -> DiagnosticsResult>; fn location(&self) -> Location; fn root_fragment(&self, object: Option<&Object>) -> Option; - fn output_type(&self) -> Option<&OutputType>; + fn output_type(&self) -> Option; fn deprecated(&self) -> Option; fn live(&self) -> Option; fn named_import(&self) -> Option; @@ -195,10 +197,10 @@ trait ResolverIr { }) } - fn directives(&self, object: Option<&Object>) -> Vec { + fn directives(&self, object: Option<&Object>, schema: &SDLSchema) -> Vec { let location = self.location(); let span = location.span(); - let mut directives = vec![self.directive(object)]; + let mut directives = vec![self.directive(object, schema)]; if let Some(deprecated) = self.deprecated() { directives.push(ConstantDirective { @@ -217,7 +219,7 @@ trait ResolverIr { directives } - fn directive(&self, object: Option<&Object>) -> ConstantDirective { + fn directive(&self, object: Option<&Object>, schema: &SDLSchema) -> ConstantDirective { let location = self.location(); let span = location.span(); let import_path = self.location().source_location().path().intern(); @@ -248,8 +250,36 @@ trait ResolverIr { arguments.push(true_argument(LIVE_ARGUMENT_NAME.0, live_field.key_location)) } - if let Some(output_type) = &self.output_type() { + if let Some(output_type) = self.output_type() { match output_type { + OutputType::Pending(type_) => { + let schema_type = schema.get_type(type_.item.inner().name.value); + let fields = match schema_type { + Some(Type::Object(id)) => { + let object = schema.object(id); + Some(&object.fields) + } + Some(Type::Interface(id)) => { + let interface = schema.interface(id); + Some(&interface.fields) + } + _ => None, + }; + let is_edge_to = fields.map_or(false, |fields| { + fields + .iter() + .any(|id| schema.field(*id).name.item == *ID_FIELD_NAME) + }); + + if !is_edge_to { + // If terse resolver does not return strong object (edge) + // it should be `@outputType` resolver + arguments.push(true_argument( + HAS_OUTPUT_TYPE_ARGUMENT_NAME.0, + type_.location, + )) + } + } OutputType::EdgeTo(_) => {} OutputType::Output(type_) => arguments.push(true_argument( HAS_OUTPUT_TYPE_ARGUMENT_NAME.0, @@ -279,7 +309,6 @@ pub struct TerseRelayResolverIr { pub type_: WithLocation, pub root_fragment: Option>, pub deprecated: Option, - pub output_type: Option, pub live: Option, pub location: Location, pub fragment_arguments: Option>, @@ -294,9 +323,12 @@ impl ResolverIr for TerseRelayResolverIr { return Ok(vec![TypeSystemDefinition::ObjectTypeExtension( ObjectTypeExtension { name: as_identifier(self.type_), - interfaces: Vec::new(), - directives: self.directives(Some(schema.object(object_id))), - fields: Some(List::generated(vec![self.field.clone()])), + interfaces: vec![], + directives: vec![], + fields: Some(List::generated(vec![FieldDefinition { + directives: self.directives(Some(schema.object(object_id)), schema), + ..self.field.clone() + }])), }, )]); } @@ -319,15 +351,20 @@ impl ResolverIr for TerseRelayResolverIr { self.location } - fn root_fragment(&self, _object: Option<&Object>) -> Option { - self.root_fragment.map(|fragment| RootFragment { - fragment, - inject_fragment_data: None, + fn root_fragment(&self, object: Option<&Object>) -> Option { + get_root_fragment_for_object(object).or_else(|| { + self.root_fragment.map(|fragment| RootFragment { + fragment, + inject_fragment_data: None, + }) }) } - fn output_type(&self) -> Option<&OutputType> { - self.output_type.as_ref() + fn output_type(&self) -> Option { + Some(OutputType::Pending(WithLocation::new( + self.location, + self.field.type_.clone(), + ))) } fn deprecated(&self) -> Option { @@ -382,7 +419,7 @@ impl ResolverIr for RelayResolverIr { Type::Object(object_id) => { let object = schema.object(object_id); self.validate_singular_implementation(schema, &object.interfaces)?; - return Ok(self.object_definitions(object)); + return Ok(self.object_definitions(object, schema)); } Type::Interface(_) => { return Err(vec![Diagnostic::error_with_data( @@ -441,35 +478,16 @@ impl ResolverIr for RelayResolverIr { } fn root_fragment(&self, object: Option<&Object>) -> Option { - if let Some(object) = object { - if object - .directives - .named(*RELAY_RESOLVER_MODEL_DIRECTIVE_NAME) - .is_some() - { - return Some(RootFragment { - fragment: WithLocation::generated(FragmentDefinitionName( - format!( - "{}__{}", - object.name.item, *RESOLVER_MODEL_INSTANCE_FIELD_NAME - ) - .intern(), - )), - inject_fragment_data: Some(FragmentDataInjectionMode::Field( - *RESOLVER_MODEL_INSTANCE_FIELD_NAME, - )), - }); - } - } - - self.root_fragment.map(|fragment| RootFragment { - fragment, - inject_fragment_data: None, + get_root_fragment_for_object(object).or_else(|| { + self.root_fragment.map(|fragment| RootFragment { + fragment, + inject_fragment_data: None, + }) }) } - fn output_type(&self) -> Option<&OutputType> { - self.output_type.as_ref() + fn output_type(&self) -> Option { + self.output_type.as_ref().cloned() } fn deprecated(&self) -> Option { @@ -511,7 +529,7 @@ impl RelayResolverIr { seen_objects: &mut HashSet, seen_interfaces: &mut HashSet, ) -> Vec { - let fields = self.fields(None); + let fields = self.fields(None, schema); // First we extend the interface itself... let mut definitions = vec![TypeSystemDefinition::InterfaceTypeExtension( InterfaceTypeExtension { @@ -526,7 +544,7 @@ impl RelayResolverIr { for object_id in &schema.interface(interface_id).implementing_objects { if !seen_objects.contains(object_id) { seen_objects.insert(*object_id); - definitions.extend(self.object_definitions(schema.object(*object_id))); + definitions.extend(self.object_definitions(schema.object(*object_id), schema)); } } @@ -594,18 +612,18 @@ impl RelayResolverIr { Ok(()) } - fn object_definitions(&self, object: &Object) -> Vec { + fn object_definitions(&self, object: &Object, schema: &SDLSchema) -> Vec { vec![TypeSystemDefinition::ObjectTypeExtension( ObjectTypeExtension { name: obj_as_identifier(object.name), interfaces: Vec::new(), directives: vec![], - fields: Some(self.fields(Some(object))), + fields: Some(self.fields(Some(object), schema)), }, )] } - fn fields(&self, object: Option<&Object>) -> List { + fn fields(&self, object: Option<&Object>, schema: &SDLSchema) -> List { let edge_to = self.output_type.as_ref().map_or_else( || { // Resolvers return arbitrary JavaScript values. However, we @@ -635,7 +653,7 @@ impl RelayResolverIr { name: self.field.name.clone(), type_: edge_to, arguments: args, - directives: self.directives(object), + directives: self.directives(object, schema), description: self.description.map(as_string_node), }]) } @@ -669,7 +687,7 @@ pub struct StrongObjectIr { } impl ResolverIr for StrongObjectIr { - fn definitions(&self, _schema: &SDLSchema) -> DiagnosticsResult> { + fn definitions(&self, schema: &SDLSchema) -> DiagnosticsResult> { let span = Span::empty(); let fields = vec![ FieldDefinition { @@ -691,7 +709,7 @@ impl ResolverIr for StrongObjectIr { name: string_key_as_identifier(*INT_TYPE), }), arguments: None, - directives: self.directives(None), + directives: self.directives(None, schema), description: None, }, ]; @@ -714,6 +732,7 @@ impl ResolverIr for StrongObjectIr { self.location } + // For Model resolver we always inject the `id` fragment fn root_fragment(&self, _: Option<&Object>) -> Option { Some(RootFragment { fragment: self.root_fragment, @@ -721,7 +740,7 @@ impl ResolverIr for StrongObjectIr { }) } - fn output_type(&self) -> Option<&OutputType> { + fn output_type(&self) -> Option { None } @@ -870,7 +889,7 @@ impl ResolverIr for WeakObjectIr { None } - fn output_type(&self) -> Option<&OutputType> { + fn output_type(&self) -> Option { None } @@ -952,3 +971,27 @@ fn dummy_token(span: &Span) -> Token { kind: TokenKind::Empty, } } + +fn get_root_fragment_for_object(object: Option<&Object>) -> Option { + if object? + .directives + .named(*RELAY_RESOLVER_MODEL_DIRECTIVE_NAME) + .is_some() + { + Some(RootFragment { + fragment: WithLocation::generated(FragmentDefinitionName( + format!( + "{}__{}", + object.unwrap().name.item, + *RESOLVER_MODEL_INSTANCE_FIELD_NAME + ) + .intern(), + )), + inject_fragment_data: Some(FragmentDataInjectionMode::Field( + *RESOLVER_MODEL_INSTANCE_FIELD_NAME, + )), + }) + } else { + None + } +} diff --git a/compiler/crates/relay-docblock/src/lib.rs b/compiler/crates/relay-docblock/src/lib.rs index e70685c627665..8e49dceadab12 100644 --- a/compiler/crates/relay-docblock/src/lib.rs +++ b/compiler/crates/relay-docblock/src/lib.rs @@ -753,10 +753,6 @@ impl RelayResolverParser { let location = type_str.location; - // TODO: Provide an output type (using a new variant) to signal that - // @outputType should be inferred from the type definition. - let output_type = None; - // These fields are subsumed by the terse syntax, and as such cannot be used with terse syntax. for forbidden_field_name in &[ *FIELD_NAME_FIELD, @@ -775,6 +771,7 @@ impl RelayResolverParser { )); } } + let named_import = self.options.use_named_imports.then_some(field.name.value); Ok(Some(TerseRelayResolverIr { field, type_: WithLocation::new(type_str.location.with_span(type_name.span), type_name.value), @@ -782,10 +779,9 @@ impl RelayResolverParser { .map(|root_fragment| root_fragment.value.map(FragmentDefinitionName)), location, deprecated, - output_type, live, fragment_arguments, - named_import: None, + named_import, })) } diff --git a/compiler/crates/relay-docblock/tests/parse/fixtures/terse-relay-resolver.expected b/compiler/crates/relay-docblock/tests/parse/fixtures/terse-relay-resolver.expected index 3cd2f650618cd..00dd82e136de8 100644 --- a/compiler/crates/relay-docblock/tests/parse/fixtures/terse-relay-resolver.expected +++ b/compiler/crates/relay-docblock/tests/parse/fixtures/terse-relay-resolver.expected @@ -61,7 +61,6 @@ TerseRelayResolver( }, ), deprecated: None, - output_type: None, live: None, location: /path/to/test/fixture/terse-relay-resolver.js:20:44, fragment_arguments: None, diff --git a/compiler/crates/relay-docblock/tests/to_schema/fixtures/terse-relay-resolver-with-output-type.expected b/compiler/crates/relay-docblock/tests/to_schema/fixtures/terse-relay-resolver-with-output-type.expected index 9fbbbd61da398..3ab3a1bd3c1fd 100644 --- a/compiler/crates/relay-docblock/tests/to_schema/fixtures/terse-relay-resolver-with-output-type.expected +++ b/compiler/crates/relay-docblock/tests/to_schema/fixtures/terse-relay-resolver-with-output-type.expected @@ -22,6 +22,6 @@ graphql` } ` ==================================== OUTPUT =================================== -extend type User @relay_resolver(import_path: "/path/to/test/fixture/terse-relay-resolver-with-output-type.js", fragment_name: "myRootFragment") { - favorite_page: ClientPage +extend type User { + favorite_page: ClientPage @relay_resolver(import_path: "/path/to/test/fixture/terse-relay-resolver-with-output-type.js", fragment_name: "myRootFragment", has_output_type: true) } diff --git a/compiler/crates/relay-docblock/tests/to_schema/fixtures/terse-relay-resolver.expected b/compiler/crates/relay-docblock/tests/to_schema/fixtures/terse-relay-resolver.expected index e61d51f0f23e7..1a5ffa5da8f7f 100644 --- a/compiler/crates/relay-docblock/tests/to_schema/fixtures/terse-relay-resolver.expected +++ b/compiler/crates/relay-docblock/tests/to_schema/fixtures/terse-relay-resolver.expected @@ -22,6 +22,6 @@ graphql` } ` ==================================== OUTPUT =================================== -extend type User @relay_resolver(import_path: "/path/to/test/fixture/terse-relay-resolver.js", fragment_name: "myRootFragment") { - favorite_page: Page +extend type User { + favorite_page: Page @relay_resolver(import_path: "/path/to/test/fixture/terse-relay-resolver.js", fragment_name: "myRootFragment") } diff --git a/packages/relay-runtime/store/__tests__/resolvers/QueryTodoModel.js b/packages/relay-runtime/store/__tests__/resolvers/QueryTodoModel.js index 2198b39feaf50..7758a67afc9d8 100644 --- a/packages/relay-runtime/store/__tests__/resolvers/QueryTodoModel.js +++ b/packages/relay-runtime/store/__tests__/resolvers/QueryTodoModel.js @@ -12,10 +12,7 @@ 'use strict'; /** - * @RelayResolver - * @onType Query - * @fieldName todo_model(todoID: ID!) - * @edgeTo TodoModel + * @RelayResolver Query.todo_model(todoID: ID!): TodoModel */ function todo_model(args: {todoID: string}): string { return args.todoID; diff --git a/packages/relay-runtime/store/__tests__/resolvers/TodoDescription.js b/packages/relay-runtime/store/__tests__/resolvers/TodoDescription.js index 31e8dbd274002..8f42fe3d08419 100644 --- a/packages/relay-runtime/store/__tests__/resolvers/TodoDescription.js +++ b/packages/relay-runtime/store/__tests__/resolvers/TodoDescription.js @@ -32,20 +32,14 @@ function createTodoDescription( } /** - * @RelayResolver - * @onType TodoDescription - * @fieldName text - * @outputType String + * @RelayResolver TodoDescription.text: String */ function text(instance: ?TodoDescription): ?string { return instance?.text; } /** - * @RelayResolver - * @onType TodoDescription - * @fieldName color - * @outputType RelayResolverValue + * @RelayResolver TodoDescription.color: RelayResolverValue */ function color(instance: ?TodoDescription): ?string { return instance?.color; diff --git a/packages/relay-runtime/store/__tests__/resolvers/TodoModel.js b/packages/relay-runtime/store/__tests__/resolvers/TodoModel.js index 54e607eab80aa..96847e9f5c4cf 100644 --- a/packages/relay-runtime/store/__tests__/resolvers/TodoModel.js +++ b/packages/relay-runtime/store/__tests__/resolvers/TodoModel.js @@ -39,19 +39,14 @@ function TodoModel(id: string): LiveState { } /** - * @RelayResolver - * @onType TodoModel - * @fieldName description + * @RelayResolver TodoModel.description: String */ function description(model: ?TodoItem): ?string { return model?.description; } /** - * @RelayResolver - * @onType TodoModel - * @fieldName fancy_description - * @outputType TodoDescription + * @RelayResolver TodoModel.fancy_description: TodoDescription */ function fancy_description(model: ?TodoItem): ?TodoDescription { if (model == null) { diff --git a/scripts/config.tests.json b/scripts/config.tests.json index 2689ab65908f8..e7b9cd8bfbd69 100644 --- a/scripts/config.tests.json +++ b/scripts/config.tests.json @@ -28,6 +28,7 @@ "enable_relay_resolver_transform": true, "use_named_imports_for_relay_resolvers": true, "relay_resolver_model_syntax_enabled": true, + "relay_resolver_enable_terse_syntax": true, "enable_flight_transform": true, "no_inline": { "kind": "enabled"