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.

Sign up for GitHub

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

do not automatically remove files not generated by Relay #3700

Closed
wants to merge 1 commit into from
+54 −2
Closed

do not automatically remove files not generated by Relay #3700

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
56 changes: 54 additions & 2 deletions compiler/crates/relay-compiler/src/build_project/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ use log::{debug, info, warn};
use rayon::{iter::IntoParallelRefIterator, slice::ParallelSlice};
use relay_codegen::Printer;
use relay_transforms::{apply_transforms, find_resolver_dependencies, DependencyMap, Programs};
use relay_typegen::TypegenLanguage;
use schema::SDLSchema;
pub use source_control::add_to_mercurial;
use std::iter::FromIterator;
Expand Down Expand Up @@ -205,6 +206,14 @@ pub fn build_programs(
Ok((programs, Arc::new(source_hashes)))
}

fn is_relay_file_path(language: &TypegenLanguage, path: &PathBuf) -> bool {
match (path.to_str(), &language) {
Copy link
Contributor

@alunyov alunyov Dec 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

do we need to convert this to_str()?

can you use this: https://doc.rust-lang.org/std/path/struct.Path.html#method.ends_with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think so, because it seems ends_with there only checks for entire chunks of the path. So that ends_with would mean the entire last chunk (the full file name in this case), which isn't what we're after. I tried using that function first. But maybe there's a better alternative still.

(None, _) => false,
(Some(path), TypegenLanguage::Flow) => path.ends_with(".graphql.js"),
(Some(path), TypegenLanguage::TypeScript) => path.ends_with(".graphql.ts"),
}
}

pub fn build_project(
config: &Config,
project_config: &ProjectConfig,
Expand Down Expand Up @@ -373,7 +382,9 @@ pub async fn commit_project(
break;
}
let path = config.root_dir.join(remaining_artifact);
config.artifact_writer.remove(path)?;
if is_relay_file_path(&project_config.typegen_config.language, &path) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is mostly nits (and not sure if it makes it better). But can we move this function to the artifact_writer?

We already have should_write:

relay/compiler/crates/relay-compiler/src/build_project/artifact_writer.rs

Line 21 in e72ea12

fn should_write(&self, path: &PathBuf, content: &[u8]) -> Result<bool, BuildProjectError>;

I think we can also add should_remove?

Copy link
Contributor

Choose a reason for hiding this comment

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

should_remove can just return a boolean instead of Result<...>.

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'd need to make sure that the typegen config is added to the artifact_writer if so, but maybe it's ok to add that? Or I could do a typegen unaware check and just check for .graphql.js or .graphql.ts always. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to do a check without typegen config.

What do you think about alternative approach, where we would filter these non-relay artifacts in the file_categorizer?

Somewhere here:

relay/compiler/crates/relay-compiler/src/file_source/file_categorizer.rs

Lines 237 to 239 in a07750f

if let Some(project_name) = self.generated_dir_mapping.find(path) {
return Ok(FileGroup::Generated { project_name });
}

config.artifact_writer.remove(path)?;
}
}
log_event.stop(delete_artifacts_time);
ArtifactMap::from(artifacts)
Expand Down Expand Up @@ -443,7 +454,9 @@ pub async fn commit_project(
if should_stop_updating_artifacts() {
break;
}
config.artifact_writer.remove(config.root_dir.join(path))?;
if is_relay_file_path(&project_config.typegen_config.language, &path) {
config.artifact_writer.remove(config.root_dir.join(path))?;
}
}
log_event.stop(delete_artifacts_incremental_time);

Expand Down Expand Up @@ -513,3 +526,42 @@ fn write_artifacts<F: Fn() -> bool + Sync + Send>(
)?;
Ok(())
}

#[cfg(test)]
mod tests {
use super::*;
#[test]
fn matches_relay_file_paths_properly() {
assert_eq!(
is_relay_file_path(
&TypegenLanguage::Flow,
&PathBuf::from("/some/folder/here/SomeModule.graphql.js")
),
true
);

assert_eq!(
is_relay_file_path(
&TypegenLanguage::TypeScript,
&PathBuf::from("/some/folder/here/SomeModule.graphql.ts")
),
true
);

assert_eq!(
is_relay_file_path(
&TypegenLanguage::Flow,
&PathBuf::from("/some/folder/here/SomeRandomFile.js")
),
false
);

assert_eq!(
is_relay_file_path(
&TypegenLanguage::TypeScript,
&PathBuf::from("/some/folder/here/SomeRandomFile.ts")
),
false
);
}
}