Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Node interface id field to be configurable. #3638

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions compiler/crates/common/src/feature_flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ pub struct FeatureFlags {

#[serde(default)]
pub enable_provided_variables: FeatureFlag,

/// The name of the `id` field that exists on the `Node` interface.
#[serde(default)]
pub node_interface_id_field: Option<StringKey>,
}

#[derive(Debug, Deserialize, Clone, Serialize)]
Expand Down
2 changes: 1 addition & 1 deletion compiler/crates/relay-codegen/tests/client_edges/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ pub fn transform_fixture(fixture: &Fixture<'_>) -> Result<String, String> {
let ir = build(&schema, &ast.definitions).unwrap();
let program = Program::from_definitions(Arc::clone(&schema), ir);
let next_program = sort_selections(
&client_edges(&program)
&client_edges(&program, None)
.and_then(|program| relay_resolvers(&program, true))
.unwrap(),
);
Expand Down
1 change: 1 addition & 0 deletions compiler/crates/relay-compiler-playground/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ fn get_programs(
base_fragment_names,
&connection_interface,
Arc::new(feature_flags),
None,
&None,
Arc::new(perf_logger),
None,
Expand Down
4 changes: 2 additions & 2 deletions compiler/crates/relay-compiler/src/build_project/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,9 @@ pub fn build_programs(
) -> Result<BuildProgramsOutput, BuildProjectFailure> {
let project_name = project_config.name;
let is_incremental_build = compiler_state.has_processed_changes()
&& !compiler_state.has_breaking_schema_change(project_name)
&& !compiler_state.has_breaking_schema_change(project_name, project_config)
&& if let Some(base) = project_config.base {
!compiler_state.has_breaking_schema_change(base)
!compiler_state.has_breaking_schema_change(base, project_config)
} else {
true
};
Expand Down
10 changes: 5 additions & 5 deletions compiler/crates/relay-compiler/src/compiler_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/

use crate::artifact_map::ArtifactMap;
use crate::config::Config;
use crate::config::{Config, ProjectConfig};
use crate::errors::{Error, Result};
use crate::file_source::{
categorize_files, extract_graphql_strings_from_file, read_file_to_string, Clock, File,
Expand Down Expand Up @@ -341,7 +341,7 @@ impl CompilerState {
.any(|sources| !sources.processed.is_empty())
}

fn is_change_safe(&self, sources: &SchemaSources) -> bool {
fn is_change_safe(&self, sources: &SchemaSources, project_config: &ProjectConfig) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add node_interface_id_field to the compiler_state itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to do this, but the config that's used to create the compiler_state is the root config, not the project config, so this seemed challenging to try to move.

let previous = sources
.get_old_sources()
.into_iter()
Expand All @@ -363,7 +363,7 @@ impl CompilerState {
&current,
&Vec::<(&str, SourceLocationKey)>::new(),
) {
Ok(schema) => schema_change.is_safe(&schema),
Ok(schema) => schema_change.is_safe(&schema, project_config.feature_flags.node_interface_id_field),
Err(_) => false,
}
}
Expand All @@ -381,14 +381,14 @@ impl CompilerState {
}

/// This method is looking at the pending schema changes to see if they may be breaking (removed types, renamed field, etc)
pub fn has_breaking_schema_change(&self, project_name: StringKey) -> bool {
pub fn has_breaking_schema_change(&self, project_name: StringKey, project_config: &ProjectConfig) -> bool {
if let Some(extension) = self.extensions.get(&project_name) {
if !extension.pending.is_empty() {
return true;
}
}
if let Some(schema) = self.schemas.get(&project_name) {
if !(schema.pending.is_empty() || self.is_change_safe(schema)) {
if !(schema.pending.is_empty() || self.is_change_safe(schema, project_config)) {
return true;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ pub fn transform_fixture(fixture: &Fixture<'_>) -> Result<String, String> {
text_artifacts: FeatureFlag::Disabled,
enable_client_edges: FeatureFlag::Enabled,
enable_provided_variables: FeatureFlag::Enabled,
node_interface_id_field: None,
};

// TODO pass base fragment names
Expand Down
4 changes: 2 additions & 2 deletions compiler/crates/relay-lsp/src/server/lsp_state_resources.rs
Original file line number Diff line number Diff line change
Expand Up @@ -342,9 +342,9 @@ impl<TPerfLogger: PerfLogger + 'static, TSchemaDocumentation: SchemaDocumentatio
.source_programs
.contains_key(&project_config.name)
&& compiler_state.has_processed_changes()
&& !compiler_state.has_breaking_schema_change(project_config.name)
&& !compiler_state.has_breaking_schema_change(project_config.name, project_config)
&& if let Some(base) = project_config.base {
!compiler_state.has_breaking_schema_change(base)
!compiler_state.has_breaking_schema_change(base, project_config)
} else {
true
};
Expand Down
13 changes: 7 additions & 6 deletions compiler/crates/relay-transforms/src/apply_transforms.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ where
let operation_program = apply_operation_transforms(
project_name,
Arc::clone(&common_program),
Arc::clone(&feature_flags),
connection_interface,
Arc::clone(&base_fragment_names),
Arc::clone(&perf_logger),
Expand Down Expand Up @@ -148,11 +149,10 @@ fn apply_common_transforms(
transform_subscriptions(&program)
})?;
program = log_event.time("transform_refetchable_fragment", || {
transform_refetchable_fragment(&program, &base_fragment_names, false)
transform_refetchable_fragment(&program, feature_flags.node_interface_id_field, &base_fragment_names, false)
})?;

program = log_event.time("client_edges", || client_edges(&program))?;

program = log_event.time("client_edges", || client_edges(&program, feature_flags.node_interface_id_field))?;
program = log_event.time("relay_resolvers", || {
relay_resolvers(&program, feature_flags.enable_relay_resolver_transform)
})?;
Expand Down Expand Up @@ -225,6 +225,7 @@ fn apply_reader_transforms(
fn apply_operation_transforms(
project_name: StringKey,
program: Arc<Program>,
feature_flags: Arc<FeatureFlags>,
connection_interface: &ConnectionInterface,
base_fragment_names: Arc<StringKeySet>,
perf_logger: Arc<impl PerfLogger>,
Expand All @@ -239,7 +240,7 @@ fn apply_operation_transforms(
program = log_event.time("split_module_import", || {
split_module_import(&program, &base_fragment_names)
});
program = log_event.time("generate_id_field", || generate_id_field(&program));
program = log_event.time("generate_id_field", || generate_id_field(&program, feature_flags.node_interface_id_field));
program = log_event.time("declarative_connection", || {
transform_declarative_connection(&program, connection_interface)
})?;
Expand Down Expand Up @@ -411,7 +412,7 @@ fn apply_typegen_transforms(
transform_subscriptions(&program)
})?;
program = log_event.time("required_directive", || required_directive(&program))?;
program = log_event.time("client_edges", || client_edges(&program))?;
program = log_event.time("client_edges", || client_edges(&program, feature_flags.node_interface_id_field))?;
program = log_event.time(
"transform_assignable_fragment_spreads_in_regular_queries",
|| transform_assignable_fragment_spreads_in_regular_queries(&program),
Expand All @@ -424,7 +425,7 @@ fn apply_typegen_transforms(
})?;
log_event.time("flatten", || flatten(&mut program, false, false))?;
program = log_event.time("transform_refetchable_fragment", || {
transform_refetchable_fragment(&program, &base_fragment_names, true)
transform_refetchable_fragment(&program, feature_flags.node_interface_id_field, &base_fragment_names, true)
})?;
program = log_event.time("remove_base_fragments", || {
remove_base_fragments(&program, base_fragment_names)
Expand Down
10 changes: 6 additions & 4 deletions compiler/crates/relay-transforms/src/client_edges.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ impl<'a> ClientEdgeMetadata<'a> {
})
}
}
pub fn client_edges(program: &Program) -> DiagnosticsResult<Program> {
let mut transform = ClientEdgesTransform::new(program);
pub fn client_edges(program: &Program, node_interface_id_field: Option<StringKey>) -> DiagnosticsResult<Program> {
let mut transform = ClientEdgesTransform::new(program, node_interface_id_field);
let mut next_program = transform
.transform_program(program)
.replace_or_else(|| program.clone());
Expand All @@ -106,12 +106,14 @@ struct ClientEdgesTransform<'program> {
new_fragments: Vec<Arc<FragmentDefinition>>,
new_operations: Vec<OperationDefinition>,
errors: Vec<Diagnostic>,
node_interface_id_field: Option<StringKey>,
}

impl<'program> ClientEdgesTransform<'program> {
fn new(program: &'program Program) -> Self {
fn new(program: &'program Program, node_interface_id_field: Option<StringKey>) -> Self {
Self {
program,
node_interface_id_field,
path: Default::default(),
query_names: Default::default(),
document_name: Default::default(),
Expand Down Expand Up @@ -169,7 +171,7 @@ impl<'program> ClientEdgesTransform<'program> {
selections,
};

let mut transformer = RefetchableFragment::new(self.program, false);
let mut transformer = RefetchableFragment::new(self.program, false, self.node_interface_id_field);

let refetchable_fragment = transformer
.transform_refetch_fragment_with_refetchable_directive(
Expand Down
10 changes: 5 additions & 5 deletions compiler/crates/relay-transforms/src/generate_id_field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ use std::{

/// A transform that adds an `id` field on any type that has an id field but
/// where there is no unaliased `id` selection.
pub fn generate_id_field(program: &Program) -> Program {
let mut transform = GenerateIDFieldTransform::new(program);
pub fn generate_id_field(program: &Program, node_interface_id_field: Option<StringKey>) -> Program {
let mut transform = GenerateIDFieldTransform::new(program, node_interface_id_field);
transform
.transform_program(program)
.replace_or_else(|| program.clone())
Expand Down Expand Up @@ -113,8 +113,8 @@ impl<'s> Transformer for GenerateIDFieldTransform<'s> {
}

impl<'s> GenerateIDFieldTransform<'s> {
fn new(program: &'s Program) -> Self {
let id_name = "id".intern();
fn new(program: &'s Program, node_interface_id_field: Option<StringKey>) -> Self {
let id_name = node_interface_id_field.unwrap_or_else(|| "id".intern());

let schema = &program.schema;
let node_interface = match schema.get_type("Node".intern()) {
Expand All @@ -124,7 +124,7 @@ impl<'s> GenerateIDFieldTransform<'s> {
.fields
.iter()
.find(|&&id| schema.field(id).name.item == id_name)
.expect("Expected `Node` to contain a field named `id`.");
.expect(&format!("Expected `Node` to contain a field named `{:}`.", id_name).to_string());

Some(NodeInterface {
id: node_interface_id,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,13 @@ use std::sync::Arc;

fn build_refetch_operation(
schema: &SDLSchema,
node_interface_id_field: Option<StringKey>,
fragment: &Arc<FragmentDefinition>,
query_name: StringKey,
variables_map: &VariableMap,
) -> DiagnosticsResult<Option<RefetchRoot>> {
let id_name = node_interface_id_field.unwrap_or_else(|| "id".intern());

if let Some(identifier_field_name) = get_fetchable_field_name(fragment, schema)? {
let identifier_field_id = get_identifier_field_id(fragment, schema, identifier_field_name)?;

Expand Down Expand Up @@ -58,7 +61,7 @@ fn build_refetch_operation(
),
});
let mut variable_definitions = build_operation_variable_definitions(&fragment);
if let Some(id_argument) = variable_definitions.named(CONSTANTS.id_name) {
if let Some(id_argument) = variable_definitions.named(id_name) {
return Err(vec![Diagnostic::error(
ValidationMessage::RefetchableFragmentOnNodeWithExistingID {
fragment_name: fragment.name.item,
Expand All @@ -67,7 +70,7 @@ fn build_refetch_operation(
)]);
}
variable_definitions.push(VariableDefinition {
name: WithLocation::new(fragment.name.location, CONSTANTS.id_name),
name: WithLocation::new(fragment.name.location, id_name),
type_: id_arg.type_.non_null(),
default_value: None,
directives: vec![],
Expand All @@ -83,7 +86,7 @@ fn build_refetch_operation(
value: WithLocation::new(
fragment.name.location,
Value::Variable(Variable {
name: WithLocation::new(fragment.name.location, CONSTANTS.id_name),
name: WithLocation::new(fragment.name.location, id_name),
type_: id_arg.type_.non_null(),
}),
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,14 @@ pub use self::refetchable_directive::REFETCHABLE_NAME;
/// Fragment to Root IR nodes.
pub fn transform_refetchable_fragment(
program: &Program,
node_interface_id_field: Option<StringKey>,
base_fragment_names: &'_ StringKeySet,
for_typegen: bool,
) -> DiagnosticsResult<Program> {
let mut next_program = Program::new(Arc::clone(&program.schema));
let query_type = program.schema.query_type().unwrap();

let mut transformer = RefetchableFragment::new(program, for_typegen);
let mut transformer = RefetchableFragment::new(program, for_typegen, node_interface_id_field);

for operation in program.operations() {
next_program.insert_operation(Arc::clone(operation));
Expand Down Expand Up @@ -102,15 +103,17 @@ pub struct RefetchableFragment<'program> {
existing_refetch_operations: ExistingRefetchOperations,
for_typegen: bool,
program: &'program Program,
node_interface_id_field: Option<StringKey>,
}

impl<'program> RefetchableFragment<'program> {
pub fn new(program: &'program Program, for_typegen: bool) -> Self {
pub fn new(program: &'program Program, for_typegen: bool, node_interface_id_field: Option<StringKey>) -> Self {
RefetchableFragment {
connection_constants: Default::default(),
existing_refetch_operations: Default::default(),
for_typegen,
program,
node_interface_id_field,
}
}

Expand Down Expand Up @@ -145,6 +148,7 @@ impl<'program> RefetchableFragment<'program> {
for generator in GENERATORS.iter() {
if let Some(refetch_root) = (generator.build_refetch_operation)(
&self.program.schema,
self.node_interface_id_field,
fragment,
refetchable_directive.query_name.item,
&variables_map,
Expand Down Expand Up @@ -294,6 +298,7 @@ impl<'program> RefetchableFragment<'program> {

type BuildRefetchOperationFn = fn(
schema: &SDLSchema,
node_interface_id_field: Option<StringKey>,
fragment: &Arc<FragmentDefinition>,
query_name: StringKey,
variables_map: &VariableMap,
Expand Down
Loading