Improve parity on ClientExtensions

Reviewed By: kassens Differential Revision: D21605964 fbshipit-source-id: 9c11b4f5aa39fc2b380014576da53b739739de3d
facebook · May 18, 2020 · e82f6c5 · e82f6c5
1 parent 26a4fc6
commit e82f6c5
Show file tree

Hide file tree

Showing 7 changed files with 86 additions and 57 deletions.
diff --git a/compiler/crates/graphql-transforms/src/client_extensions.rs b/compiler/crates/graphql-transforms/src/client_extensions.rs
@@ -14,6 +14,7 @@ use graphql_ir::{
 };
 use interner::Intern;
 use interner::StringKey;
+use lazy_static::lazy_static;
 use std::sync::Arc;
 
 /// A transform that group all client selections and generates ... @__clientExtension inline fragments
@@ -28,20 +29,11 @@ pub fn client_extensions<'s>(program: &Program<'s>) -> Program<'s> {
 
 type Seen = FnvHashMap<PointerAddress, Transformed<Selection>>;
 
-pub struct ClientExtensionConstants {
-    pub client_extension_directive_name: StringKey,
-}
-
-impl Default for ClientExtensionConstants {
-    fn default() -> Self {
-        Self {
-            client_extension_directive_name: "__clientExtension".intern(),
-        }
-    }
+lazy_static! {
+    pub static ref CLIENT_EXTENSION_DIRECTIVE_NAME: StringKey = "__clientExtension".intern();
 }
 
 struct ClientExtensionsTransform<'s> {
-    client_extension_constants: ClientExtensionConstants,
     program: &'s Program<'s>,
     empty_location: Location,
     seen: Seen,
@@ -50,7 +42,6 @@ struct ClientExtensionsTransform<'s> {
 impl<'s> ClientExtensionsTransform<'s> {
     fn new(program: &'s Program<'s>) -> Self {
         Self {
-            client_extension_constants: Default::default(),
             program,
             seen: Default::default(),
             empty_location: Location::new(FileKey::new(""), Span::new(0, 0)),
@@ -63,8 +54,7 @@ impl<'s> ClientExtensionsTransform<'s> {
             name: WithLocation::new(
                 // The directive is only used at codegen step, location is not necessary
                 self.empty_location,
-                self.client_extension_constants
-                    .client_extension_directive_name,
+                *CLIENT_EXTENSION_DIRECTIVE_NAME,
             ),
             arguments: Default::default(),
         }

diff --git a/compiler/crates/graphql-transforms/src/flatten.rs b/compiler/crates/graphql-transforms/src/flatten.rs
@@ -5,19 +5,15 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-use crate::inline_data_fragment::INLINE_DATA_CONSTANTS;
-use crate::match_::MATCH_CONSTANTS;
-use crate::util::PointerAddress;
+use crate::util::{is_relay_custom_inline_fragment_directive, PointerAddress};
 use graphql_ir::{
     Condition, Directive, FragmentDefinition, InlineFragment, LinkedField, OperationDefinition,
     Program, Selection,
 };
 use schema::TypeReference;
 
 use crate::node_identifier::NodeIdentifier;
-use fnv::{FnvHashMap, FnvHashSet};
-use interner::{Intern, StringKey};
-use lazy_static::lazy_static;
+use fnv::FnvHashMap;
 use std::sync::Arc;
 
 type SeenLinkedFields = FnvHashMap<PointerAddress, Arc<LinkedField>>;
@@ -268,22 +264,11 @@ impl<'s> FlattenTransform<'s> {
     }
 }
 
-lazy_static! {
-    static ref RELAY_INLINE_FRAGMENT_DIRECTIVES: FnvHashSet<StringKey> = vec![
-        "__clientExtension".intern(),
-        "defer".intern(),
-        MATCH_CONSTANTS.custom_module_directive_name,
-        INLINE_DATA_CONSTANTS.internal_directive_name,
-    ]
-    .into_iter()
-    .collect();
-}
-
 fn should_flatten_inline_with_directives(directives: &[Directive], is_for_codegen: bool) -> bool {
     if is_for_codegen {
         !directives
             .iter()
-            .any(|directive| RELAY_INLINE_FRAGMENT_DIRECTIVES.contains(&directive.name.item))
+            .any(is_relay_custom_inline_fragment_directive)
     } else {
         directives.is_empty()
     }

diff --git a/compiler/crates/graphql-transforms/src/skip_redundant_nodes.rs b/compiler/crates/graphql-transforms/src/skip_redundant_nodes.rs
@@ -5,8 +5,11 @@
  * LICENSE file in the root directory of this source tree.
  */
 
+use crate::client_extensions::CLIENT_EXTENSION_DIRECTIVE_NAME;
 use crate::node_identifier::NodeIdentifier;
 use crate::util::PointerAddress;
+
+use common::NamedItem;
 use fnv::{FnvBuildHasher, FnvHashMap};
 use graphql_ir::{
     Condition, FragmentDefinition, InlineFragment, LinkedField, OperationDefinition, Program,
@@ -200,6 +203,21 @@ impl<'s> SkipRedundantNodesTransform<'s> {
                 if let Some(Some(existing_selection_map)) = selection_map.0.get_mut(&identifier) {
                     self.transform_inline_fragment(selection, existing_selection_map)
                         .map(Selection::InlineFragment)
+                } else if selection
+                    .directives
+                    .named(*CLIENT_EXTENSION_DIRECTIVE_NAME)
+                    .is_some()
+                {
+                    let mut linked_selection_map = Default::default();
+                    let result = self
+                        .transform_inline_fragment(selection, &mut linked_selection_map)
+                        .map(Selection::InlineFragment);
+                    if !linked_selection_map.0.is_empty() {
+                        selection_map
+                            .0
+                            .insert(identifier, Some(linked_selection_map));
+                    }
+                    result
                 } else {
                     // Fork for inline fragments for the same reason
                     let mut next_selection_map = selection_map.clone();
@@ -374,10 +392,8 @@ impl<'s> Transformer for SkipRedundantNodesTransform<'s> {
  */
 fn get_partitioned_selections(selections: &[Selection]) -> Vec<&Selection> {
     let (mut left, right): (Vec<_>, Vec<_>) = selections.iter().partition(|sel| match sel {
-        Selection::LinkedField(_) | Selection::ScalarField(_) | Selection::FragmentSpread(_) => {
-            true
-        }
-        Selection::Condition(_) | Selection::InlineFragment(_) => false,
+        Selection::LinkedField(_) | Selection::ScalarField(_) => true,
+        _ => false,
     });
     left.extend(right);
     left

diff --git a/compiler/crates/graphql-transforms/src/util.rs b/compiler/crates/graphql-transforms/src/util.rs
@@ -5,15 +5,18 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-use crate::client_extensions::ClientExtensionConstants;
+use crate::client_extensions::CLIENT_EXTENSION_DIRECTIVE_NAME;
 use crate::connections::ConnectionConstants;
 use crate::handle_fields::HandleFieldConstants;
+use crate::inline_data_fragment::INLINE_DATA_CONSTANTS;
 use crate::match_::MATCH_CONSTANTS;
 use crate::refetchable_fragment::CONSTANTS as REFETCHABLE_CONSTANTS;
 use crate::INTERNAL_METADATA_DIRECTIVE;
 
+use fnv::FnvHashSet;
 use graphql_ir::{Argument, Directive, Value};
-use interner::StringKey;
+use interner::{Intern, StringKey};
+use lazy_static::lazy_static;
 
 // A wrapper type that allows comparing pointer equality of references. Two
 // `PointerAddress` values are equal if they point to the same memory location.
@@ -74,15 +77,13 @@ pub fn extract_variable_name(argument: Option<&Argument>) -> Option<StringKey> {
 }
 
 pub struct CustomMetadataDirectives {
-    client_extension_constants: ClientExtensionConstants,
     connection_constants: ConnectionConstants,
     handle_field_constants: HandleFieldConstants,
 }
 
 impl Default for CustomMetadataDirectives {
     fn default() -> Self {
         Self {
-            client_extension_constants: ClientExtensionConstants::default(),
             connection_constants: ConnectionConstants::default(),
             handle_field_constants: HandleFieldConstants::default(),
         }
@@ -91,9 +92,7 @@ impl Default for CustomMetadataDirectives {
 
 impl CustomMetadataDirectives {
     pub fn is_custom_metadata_directive(&self, name: StringKey) -> bool {
-        name == self
-            .client_extension_constants
-            .client_extension_directive_name
+        name == *CLIENT_EXTENSION_DIRECTIVE_NAME
             || name == self.connection_constants.connection_metadata_directive_name
             || name == self.handle_field_constants.handle_field_directive_name
             || name == MATCH_CONSTANTS.custom_module_directive_name
@@ -102,3 +101,18 @@ impl CustomMetadataDirectives {
             || name == *INTERNAL_METADATA_DIRECTIVE
     }
 }
+
+lazy_static! {
+    static ref RELAY_CUSTOM_INLINE_FRAGMENT_DIRECTIVES: FnvHashSet<StringKey> = vec![
+        *CLIENT_EXTENSION_DIRECTIVE_NAME,
+        MATCH_CONSTANTS.custom_module_directive_name,
+        INLINE_DATA_CONSTANTS.internal_directive_name,
+        "defer".intern(),
+    ]
+    .into_iter()
+    .collect();
+}
+
+pub fn is_relay_custom_inline_fragment_directive(directive: &Directive) -> bool {
+    RELAY_CUSTOM_INLINE_FRAGMENT_DIRECTIVES.contains(&directive.name.item)
+}
diff --git a/compiler/crates/relay-compiler/src/build_project/apply_transforms.rs b/compiler/crates/relay-compiler/src/build_project/apply_transforms.rs
@@ -135,6 +135,7 @@ fn apply_reader_transforms<'schema>(
     let log_event = perf_logger.create_event("apply_reader_transforms");
     log_event.string("project", project_name.to_string());
 
+    let program = log_event.time("client_extensions", || client_extensions(&program));
     let program = log_event.time("handle_field_transform", || {
         handle_field_transform(&program)
     });
@@ -144,7 +145,6 @@ fn apply_reader_transforms<'schema>(
     });
     let program = log_event.time("flatten", || flatten(&program, true));
     let program = log_event.time("skip_redundant_nodes", || skip_redundant_nodes(&program));
-    let program = log_event.time("client_extensions", || client_extensions(&program));
 
     perf_logger.complete_event(log_event);
 
@@ -219,12 +219,12 @@ fn apply_normalization_transforms<'schema>(
         validate_server_only_directives(&program)
     })?;
     let program = log_event.time("inline_fragments", || inline_fragments(&program));
+    let program = log_event.time("client_extensions", || client_extensions(&program));
     let program = log_event.time("flatten", || flatten(&program, true));
     log_event.time("validate_module_conflicts", || {
         validate_module_conflicts(&program)
     })?;
     let program = log_event.time("skip_redundant_nodes", || skip_redundant_nodes(&program));
-    let program = log_event.time("client_extensions", || client_extensions(&program));
 
     let program = log_event.time("generate_typename", || generate_typename(&program));
     perf_logger.complete_event(log_event);

diff --git a/...r/crates/relay-compiler/tests/compile_relay_artifacts/fixtures/client-conditions.expected b/...r/crates/relay-compiler/tests/compile_relay_artifacts/fixtures/client-conditions.expected
@@ -82,6 +82,18 @@ interface ClientNamed {
           "name": "name",
           "storageKey": null
         },
+        {
+          "kind": "ClientExtension",
+          "selections": [
+            {
+              "alias": null,
+              "args": null,
+              "kind": "ScalarField",
+              "name": "client_nickname",
+              "storageKey": null
+            }
+          ]
+        },
         {
           "kind": "InlineFragment",
           "selections": [
@@ -136,18 +148,6 @@ interface ClientNamed {
           ],
           "type": "Actor",
           "abstractKey": "__isActor"
-        },
-        {
-          "kind": "ClientExtension",
-          "selections": [
-            {
-              "alias": null,
-              "args": null,
-              "kind": "ScalarField",
-              "name": "client_nickname",
-              "storageKey": null
-            }
-          ]
         }
       ]
     },

diff --git a/...ates/relay-compiler/tests/compile_relay_artifacts/fixtures/relay-client-id-field.expected b/...ates/relay-compiler/tests/compile_relay_artifacts/fixtures/relay-client-id-field.expected
@@ -171,6 +171,18 @@ query RelayClientIDFieldQuery($id: ID!) {
                           }
                         ],
                         "storageKey": null
+                      },
+                      {
+                        "kind": "ClientExtension",
+                        "selections": [
+                          {
+                            "alias": null,
+                            "args": null,
+                            "kind": "ScalarField",
+                            "name": "__id",
+                            "storageKey": null
+                          }
+                        ]
                       }
                     ],
                     "type": "PlainCommentBody",
@@ -369,6 +381,18 @@ query RelayClientIDFieldQuery($id: ID!) {
                           }
                         ],
                         "storageKey": null
+                      },
+                      {
+                        "kind": "ClientExtension",
+                        "selections": [
+                          {
+                            "alias": null,
+                            "args": null,
+                            "kind": "ScalarField",
+                            "name": "__id",
+                            "storageKey": null
+                          }
+                        ]
                       }
                     ],
                     "type": "PlainCommentBody",