Skip to content

Add feature flag for fragment arguments #4648

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

Closed
Show file tree
Hide file tree
Changes from 1 commit
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
5 changes: 5 additions & 0 deletions compiler/crates/common/src/feature_flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,11 @@ pub struct FeatureFlags {
/// excludes resolver metadata from reader ast
#[serde(default)]
pub disable_resolver_reader_ast: bool,

/// Add support for parsing and transforming variable definitions on fragment
/// definitions and arguments on fragment spreads.
#[serde(default)]
pub enable_fragment_argument_transform: bool,
}

#[derive(Debug, Deserialize, Clone, Serialize, Default)]
Expand Down
22 changes: 11 additions & 11 deletions compiler/crates/graphql-syntax/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,17 @@ pub fn parse_executable(
parse_executable_with_error_recovery(source, source_location).into()
}

/// Parses a GraphQL document that's restricted to executable
/// definitions with custom feature flags passed as `features`.
pub fn parse_executable_with_features(
source: &str,
source_location: SourceLocationKey,
features: ParserFeatures,
) -> DiagnosticsResult<ExecutableDocument> {
parse_executable_with_error_recovery_and_parser_features(source, source_location, features)
.into()
}

/// Parses a GraphQL document that's restricted to executable definitions,
/// with error recovery.
pub fn parse_executable_with_error_recovery(
Expand All @@ -81,17 +92,6 @@ pub fn parse_executable_with_error_recovery_and_parser_features(
parser.parse_executable_document()
}

/// Parses a GraphQL document that's restricted to executable
/// definitions with custom feature flags passed as `features`.
pub fn parse_executable_with_features(
source: &str,
source_location: SourceLocationKey,
features: ParserFeatures,
) -> DiagnosticsResult<ExecutableDocument> {
parse_executable_with_error_recovery_and_parser_features(source, source_location, features)
.into()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a meaningful change, or is the function just moving?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just moving the function around. The grouping felt weird.


/// Parses a GraphQL document that's restricted to type system definitions
/// including schema definition, type definitions and type system extensions.
pub fn parse_schema_document(
Expand Down
4 changes: 2 additions & 2 deletions compiler/crates/graphql-syntax/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,15 @@ use crate::syntax_error::SyntaxError;

type ParseResult<T> = Result<T, ()>;

#[derive(Default, PartialEq)]
#[derive(Default, Clone, Copy, PartialEq)]
pub enum FragmentArgumentSyntaxKind {
#[default]
None,
OnlyFragmentVariableDefinitions,
SpreadArgumentsAndFragmentVariableDefinitions,
}

#[derive(Default)]
#[derive(Default, Clone, Copy)]
pub struct ParserFeatures {
/// Whether and how to enable the experimental fragment variables definitions syntax
pub fragment_argument_capability: FragmentArgumentSyntaxKind,
Expand Down
1 change: 1 addition & 0 deletions compiler/crates/relay-compiler/src/compiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@ async fn build_projects<TPerfLogger: PerfLogger + 'static>(
GraphQLAsts::from_graphql_sources_map(
&compiler_state.graphql_sources,
&dirty_artifact_sources,
&config,
)
})?;

Expand Down
20 changes: 17 additions & 3 deletions compiler/crates/relay-compiler/src/graphql_asts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use std::collections::hash_map::Entry;
use std::collections::HashSet;
use std::path::Path;
use std::path::PathBuf;
use std::sync::Arc;

use common::Diagnostic;
use common::SourceLocationKey;
Expand All @@ -18,13 +19,16 @@ use graphql_ir::ExecutableDefinitionName;
use graphql_ir::FragmentDefinitionName;
use graphql_ir::OperationDefinitionName;
use graphql_syntax::ExecutableDefinition;
use relay_config::ProjectConfig;
use relay_config::ProjectName;

use crate::artifact_map::ArtifactSourceKey;
use crate::compiler_state::GraphQLSources;
use crate::config::Config;
use crate::errors::Error;
use crate::errors::Result;
use crate::file_source::LocatedGraphQLSource;
use crate::utils::get_parser_features;

#[derive(Debug)]
pub struct GraphQLAsts {
Expand All @@ -50,10 +54,13 @@ impl GraphQLAsts {
pub fn from_graphql_sources_map(
graphql_sources_map: &FnvHashMap<ProjectName, GraphQLSources>,
dirty_artifact_sources: &FnvHashMap<ProjectName, Vec<ArtifactSourceKey>>,
config: &Arc<Config>,
) -> Result<FnvHashMap<ProjectName, GraphQLAsts>> {
graphql_sources_map
.iter()
.map(|(&project_name, sources)| {
let project_config = &config.projects[&project_name];

let asts = GraphQLAsts::from_graphql_sources(
sources,
dirty_artifact_sources
Expand All @@ -73,6 +80,7 @@ impl GraphQLAsts {
})
.collect()
}),
project_config,
)?;
Ok((project_name, asts))
})
Expand All @@ -85,7 +93,10 @@ impl GraphQLAsts {
pub fn from_graphql_sources(
graphql_sources: &GraphQLSources,
dirty_definitions: Option<Vec<&ExecutableDefinitionName>>,
project_config: &ProjectConfig,
) -> Result<Self> {
let parser_features = get_parser_features(project_config);

let mut syntax_errors = Vec::new();

let mut asts: FnvHashMap<PathBuf, Vec<ExecutableDefinition>> = Default::default();
Expand All @@ -108,9 +119,10 @@ impl GraphQLAsts {
{
let source_location =
SourceLocationKey::embedded(&file_name.to_string_lossy(), *index);
match graphql_syntax::parse_executable(
match graphql_syntax::parse_executable_with_features(
&graphql_source.text_source().text,
source_location,
parser_features,
) {
Ok(document) => {
for def in &document.definitions {
Expand Down Expand Up @@ -145,9 +157,10 @@ impl GraphQLAsts {
// TODO: parse name instead of the whole graphql text
let source_location =
SourceLocationKey::embedded(&file_name.to_string_lossy(), *index);
if let Ok(document) = graphql_syntax::parse_executable(
if let Ok(document) = graphql_syntax::parse_executable_with_features(
&graphql_source.text_source().text,
source_location,
parser_features,
) {
for def in document.definitions {
match def {
Expand Down Expand Up @@ -210,9 +223,10 @@ impl GraphQLAsts {
{
let source_location =
SourceLocationKey::embedded(&file_name.to_string_lossy(), *index);
match graphql_syntax::parse_executable(
match graphql_syntax::parse_executable_with_features(
&graphql_source.text_source().text,
source_location,
parser_features,
) {
Ok(document) => {
definitions_for_file.extend(document.definitions);
Expand Down
2 changes: 2 additions & 0 deletions compiler/crates/relay-compiler/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ mod operation_persister;
mod red_to_green;
pub mod saved_state;
pub mod status_reporter;
mod utils;

pub use artifact_map::ArtifactSourceKey;
pub use build_project::add_to_mercurial;
Expand Down Expand Up @@ -72,3 +73,4 @@ pub use graphql_asts::GraphQLAsts;
pub use operation_persister::LocalPersister;
pub use operation_persister::RemotePersister;
pub use relay_config::ProjectName;
pub use utils::get_parser_features;
16 changes: 16 additions & 0 deletions compiler/crates/relay-compiler/src/utils.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
use graphql_syntax::FragmentArgumentSyntaxKind;
use graphql_syntax::ParserFeatures;
use relay_config::ProjectConfig;

pub fn get_parser_features(project_config: &ProjectConfig) -> ParserFeatures {
ParserFeatures {
fragment_argument_capability: if project_config
.feature_flags
.enable_fragment_argument_transform
{
FragmentArgumentSyntaxKind::SpreadArgumentsAndFragmentVariableDefinitions
} else {
FragmentArgumentSyntaxKind::None
},
}
}
9 changes: 7 additions & 2 deletions compiler/crates/relay-lsp/src/graphql_tools/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,14 @@ use graphql_ir::OperationDefinition;
use graphql_ir::OperationDefinitionName;
use graphql_ir::Program;
use graphql_ir::Selection;
use graphql_syntax::parse_executable_with_error_recovery;
use graphql_syntax::parse_executable_with_error_recovery_and_parser_features;
use graphql_text_printer::print_full_operation;
use intern::string_key::Intern;
use intern::string_key::StringKey;
use lsp_types::request::Request;
use lsp_types::Url;
use relay_compiler::config::ProjectConfig;
use relay_compiler::get_parser_features;
use relay_compiler::ProjectName;
use relay_transforms::apply_transforms;
use relay_transforms::CustomTransformsConfig;
Expand Down Expand Up @@ -229,7 +230,11 @@ pub(crate) fn get_query_text<
))
})?;

let result = parse_executable_with_error_recovery(&original_text, SourceLocationKey::Generated);
let result = parse_executable_with_error_recovery_and_parser_features(
&original_text,
SourceLocationKey::Generated,
get_parser_features(project_config),
);

if !&result.diagnostics.is_empty() {
return Err(LSPRuntimeError::UnexpectedError(
Expand Down
22 changes: 15 additions & 7 deletions compiler/crates/relay-lsp/src/server/lsp_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use graphql_ir::BuilderOptions;
use graphql_ir::FragmentVariablesSemantic;
use graphql_ir::Program;
use graphql_ir::RelayMode;
use graphql_syntax::parse_executable_with_error_recovery;
use graphql_syntax::parse_executable_with_error_recovery_and_parser_features;
use graphql_syntax::ExecutableDefinition;
use graphql_syntax::ExecutableDocument;
use intern::string_key::Intern;
Expand All @@ -35,6 +35,7 @@ use lsp_types::Range;
use lsp_types::TextDocumentPositionParams;
use lsp_types::Url;
use relay_compiler::config::Config;
use relay_compiler::get_parser_features;
use relay_compiler::FileCategorizer;
use relay_compiler::ProjectName;
use relay_docblock::parse_docblock_ast;
Expand Down Expand Up @@ -220,6 +221,11 @@ impl<TPerfLogger: PerfLogger + 'static, TSchemaDocumentation: SchemaDocumentatio
LSPRuntimeError::UnexpectedError(format!("Expected GraphQL sources for URL {}", url))
})?;
let project_name = self.extract_project_name_from_url(url)?;
let project_config = self
.config
.projects
.get(&ProjectName::from(project_name))
.unwrap();
let schema = self
.schemas
.get(&project_name)
Expand All @@ -233,9 +239,10 @@ impl<TPerfLogger: PerfLogger + 'static, TSchemaDocumentation: SchemaDocumentatio

match feature {
JavaScriptSourceFeature::GraphQL(graphql_source) => {
let result = parse_executable_with_error_recovery(
let result = parse_executable_with_error_recovery_and_parser_features(
&graphql_source.text_source().text,
source_location_key,
get_parser_features(project_config),
);
diagnostics.extend(result.diagnostics.iter().map(|diagnostic| {
self.diagnostic_reporter
Expand Down Expand Up @@ -282,11 +289,6 @@ impl<TPerfLogger: PerfLogger + 'static, TSchemaDocumentation: SchemaDocumentatio
}
}

let project_config = self
.config
.projects
.get(&ProjectName::from(project_name))
.unwrap();
for (index, docblock_source) in docblock_sources.iter().enumerate() {
let source_location_key = SourceLocationKey::embedded(url.as_ref(), index);
let text_source = docblock_source.text_source();
Expand Down Expand Up @@ -485,9 +487,15 @@ impl<TPerfLogger: PerfLogger + 'static, TSchemaDocumentation: SchemaDocumentatio
&self,
text_document_uri: &Url,
) -> LSPRuntimeResult<Vec<ExecutableDefinition>> {
let project_name: ProjectName = self
.extract_project_name_from_url(&text_document_uri)?
.into();
let project_config = self.config.projects.get(&project_name).unwrap();

extract_executable_definitions_from_text_document(
text_document_uri,
&self.synced_javascript_features,
get_parser_features(project_config),
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ impl<TPerfLogger: PerfLogger + 'static, TSchemaDocumentation: SchemaDocumentatio
GraphQLAsts::from_graphql_sources_map(
&compiler_state.graphql_sources,
&compiler_state.get_dirty_artifact_sources(&self.lsp_state.config),
&self.lsp_state.config,
)
})?;

Expand Down
20 changes: 15 additions & 5 deletions compiler/crates/relay-lsp/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@ use common::TextSource;
use dashmap::DashMap;
use docblock_syntax::parse_docblock;
use extract_graphql::JavaScriptSourceFeature;
use graphql_syntax::parse_executable_with_error_recovery;
use graphql_syntax::parse_executable_with_error_recovery_and_parser_features;
use graphql_syntax::ExecutableDefinition;
use graphql_syntax::ParserFeatures;
use intern::string_key::StringKey;
use log::debug;
use lsp_types::Position;
use lsp_types::TextDocumentPositionParams;
use lsp_types::Url;
use relay_compiler::get_parser_features;
use relay_compiler::FileCategorizer;
use relay_compiler::FileGroup;
use relay_compiler::ProjectConfig;
Expand All @@ -42,6 +44,7 @@ pub fn is_file_uri_in_dir(root_dir: PathBuf, file_uri: &Url) -> bool {
pub fn extract_executable_definitions_from_text_document(
text_document_uri: &Url,
source_feature_cache: &DashMap<Url, Vec<JavaScriptSourceFeature>>,
parser_features: ParserFeatures,
) -> LSPRuntimeResult<Vec<ExecutableDefinition>> {
let source_features = source_feature_cache
.get(text_document_uri)
Expand All @@ -56,9 +59,10 @@ pub fn extract_executable_definitions_from_text_document(
JavaScriptSourceFeature::GraphQL(graphql_source) => Some(graphql_source),
})
.map(|graphql_source| {
let document = parse_executable_with_error_recovery(
let document = parse_executable_with_error_recovery_and_parser_features(
&graphql_source.text_source().text,
SourceLocationKey::standalone(&text_document_uri.to_string()),
parser_features,
)
.item;

Expand Down Expand Up @@ -134,11 +138,14 @@ pub fn extract_feature_from_text(

let source_location_key = SourceLocationKey::embedded(uri.as_ref(), index);

let parser_features = get_parser_features(project_config);

match javascript_feature {
JavaScriptSourceFeature::GraphQL(graphql_source) => {
let document = parse_executable_with_error_recovery(
let document = parse_executable_with_error_recovery_and_parser_features(
&graphql_source.text_source().text,
source_location_key,
parser_features,
)
.item;

Expand All @@ -163,8 +170,11 @@ pub fn extract_feature_from_text(
Ok((Feature::GraphQLDocument(document), position_span))
}
JavaScriptSourceFeature::Docblock(docblock_source) => {
let executable_definitions_in_file =
extract_executable_definitions_from_text_document(uri, source_feature_cache)?;
let executable_definitions_in_file = extract_executable_definitions_from_text_document(
uri,
source_feature_cache,
parser_features,
)?;

let text_source = &docblock_source.text_source();
let text = &text_source.text;
Expand Down