Merge branch 'main' into bandaid-error-on-hot-module-replacement

.github/workflows/ci.yml

@@ -176,8 +176,8 @@ jobs:
       - name: 'Check working directory status'
         run: './scripts/check-git-status.sh'
 
-  rustfmt:
-    name: Run cargo fmt and clippy
+  cargo-fmt:
+    name: cargo fmt
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v3
@@ -188,11 +188,22 @@ jobs:
           components: rustfmt
       - name: Run cargo fmt
         run: cargo fmt
-      - name: Run cargo clippy
-        run: cargo clippy
       - name: 'Check working directory status'
         run: './scripts/check-git-status.sh'
 
+  cargo-clippy:
+    name: cargo clippy
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - name: Install Rust
+        uses: actions-rust-lang/setup-rust-toolchain@v1
+        with:
+          toolchain: stable
+          components: rustfmt
+      - name: Run cargo clippy
+        run: cargo clippy
+
   cargo-test:
     name: Run cargo test (excluding relay tests)
     runs-on: ubuntu-latest
@@ -225,7 +236,8 @@ jobs:
         build-demos,
         build-website,
         prettier,
-        rustfmt,
+        cargo-fmt,
+        cargo-clippy,
         cargo-test,
         typecheck-demos,
       ]

crates/common_lang_types/Cargo.toml

@@ -9,3 +9,4 @@ license = { workspace = true }
 intern = { path = "../../relay-crates/intern" }
 string_key_newtype = { path = "../string_key_newtype" }
 serde = { workspace = true }
+pathdiff = { workspace = true }

crates/common_lang_types/src/location.rs

@@ -1,8 +1,11 @@
-use std::{error::Error, fmt};
+use std::{error::Error, fmt, path::PathBuf};
 
-use intern::Lookup;
+use intern::string_key::{Intern, Lookup};
 
-use crate::{text_with_carats::text_with_carats, RelativePathToSourceFile, Span, WithSpan};
+use crate::{
+    text_with_carats::text_with_carats, CurrentWorkingDirectory, RelativePathToSourceFile, Span,
+    WithSpan,
+};
 
 /// A source, which consists of a path from the config's project root
 /// to the source file, and an optional span indicating the subset of
@@ -12,22 +15,54 @@ use crate::{text_with_carats::text_with_carats, RelativePathToSourceFile, Span,
 /// as this will probably mean that sources are more reusable
 /// during watch mode.
 #[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
-pub struct RelativeTextSource {
-    pub path: RelativePathToSourceFile,
+pub struct TextSource {
+    pub current_working_directory: CurrentWorkingDirectory,
+    pub relative_path_to_source_file: RelativePathToSourceFile,
     pub span: Option<Span>,
 }
 
-impl RelativeTextSource {
-    pub fn read_to_string(&self) -> (&str, String) {
+const ISO_PRINT_ABSOLUTE_FILEPATH: &str = "ISO_PRINT_ABSOLUTE_FILEPATH";
+
+impl TextSource {
+    pub fn read_to_string(&self) -> (String, String) {
         // TODO maybe intern these or somehow avoid reading a bajillion times.
         // This is especially important for when we display many errors.
-        let file_path = self.path.lookup();
-        let file_contents = std::fs::read_to_string(file_path).expect("file should exist");
+        let mut file_path = PathBuf::from(self.current_working_directory.lookup());
+        let relative_path = self.relative_path_to_source_file.lookup();
+        file_path.push(relative_path);
+
+        // HACK
+        //
+        // When we run pnpm build-pet-demo (etc), then the terminal's working directory is
+        // the isograph folder. But the process thinks that the working directory is
+        // /demos/pet-demo. As a result, if we print relative paths, we can't command-click
+        // on them, leading to a worse developer experience when working on Isograph.
+        //
+        // On the other hand, printing relative paths (from the current working directory):
+        // - is a nice default
+        // - means that if we capture that output, e.g. for fixtures, we can have consistent
+        //   fixture output, no matter what machine the fixtures were generated on.
+        //
+        // So, we need both options. This can probably be improved somewhat.
+        let absolute_or_relative_file_path = if std::env::var(ISO_PRINT_ABSOLUTE_FILEPATH).is_ok() {
+            file_path
+                .to_str()
+                .expect("Expected path to be able to be stringified.")
+                .to_string()
+        } else {
+            relative_path.to_string()
+        };
+
+        let file_contents =
+            std::fs::read_to_string(&absolute_or_relative_file_path).expect("file should exist");
         if let Some(span) = self.span {
             // TODO we're cloning here unnecessarily, I think!
-            (file_path, file_contents[span.as_usize_range()].to_string())
+            (
+                absolute_or_relative_file_path,
+                file_contents[span.as_usize_range()].to_string(),
+            )
         } else {
-            (file_path, file_contents)
+            (absolute_or_relative_file_path, file_contents)
         }
     }
 }
@@ -202,3 +237,18 @@ impl<T> From<WithEmbeddedRelativeLocation<T>> for WithLocation<T> {
         }
     }
 }
+
+pub fn relative_path_from_absolute_and_working_directory(
+    current_working_directory: CurrentWorkingDirectory,
+    absolute_path: &PathBuf,
+) -> RelativePathToSourceFile {
+    pathdiff::diff_paths(
+        absolute_path,
+        PathBuf::from(current_working_directory.lookup()),
+    )
+    .expect("Expected path to be diffable")
+    .to_str()
+    .expect("Expected path to be able to be stringified")
+    .intern()
+    .into()
+}

crates/common_lang_types/src/string_key_types.rs

@@ -1,5 +1,7 @@
 pub use string_key_newtype::StringKeyNewtype;
-use string_key_newtype::{string_key_conversion, string_key_newtype};
+use string_key_newtype::{
+    string_key_conversion, string_key_newtype, string_key_newtype_no_display,
+};
 
 string_key_newtype!(DirectiveName);
 string_key_newtype!(DirectiveArgumentName);
@@ -111,9 +113,35 @@ string_key_newtype!(IsographDirectiveName);
 
 string_key_newtype!(FieldArgumentName);
 
-// This path is from the config's project root to the source file.
-string_key_newtype!(RelativePathToSourceFile);
-
 string_key_newtype!(ArtifactFileType);
 
 string_key_newtype!(JavascriptVariableName);
+
+// HACK:
+// Locations contain two paths:
+// - The absolute path to where the compiler is executed (i.e. the current
+//   working directory)
+// - The relative path from the working directory to the source file
+//
+// These are separate paths because the CurrentWorkingDirectory has a
+// std::fmt::Display impl that prints the hard-coded string
+// "CurrentWorkingDirectory", so that generated fixtures (which include
+// debug output of the CurrentWorkingDirectory) are consistent when we
+// generated from multiple machines (including on arbitrary CI machines).
+//
+// We print the relative path in error messages, but use the full path
+// (i.e. cwd + relative path) for reading the content of the source file
+// for printing errors.
+string_key_newtype_no_display!(CurrentWorkingDirectory);
+impl std::fmt::Display for CurrentWorkingDirectory {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "CurrentWorkingDirectory")
+    }
+}
+impl std::fmt::Debug for CurrentWorkingDirectory {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "CurrentWorkingDirectory")
+    }
+}
+
+string_key_newtype!(RelativePathToSourceFile);

crates/isograph_cli/Cargo.toml

@@ -5,6 +5,8 @@ edition = { workspace = true }
 license = { workspace = true }
 
 [dependencies]
+common_lang_types = { path = "../common_lang_types" }
+intern = { path = "../../relay-crates/intern" }
 isograph_compiler = { path = "../isograph_compiler" }
 isograph_config = { path = "../isograph_config" }
 isograph_lsp = { path = "../isograph_lsp" }

crates/isograph_cli/src/main.rs

@@ -2,6 +2,8 @@ mod opt;
 
 use clap::Parser;
 use colored::Colorize;
+use common_lang_types::CurrentWorkingDirectory;
+use intern::string_key::Intern;
 use isograph_compiler::{compile_and_print, handle_watch_command};
 use isograph_config::create_config;
 use opt::{Command, CompileCommand, LspCommand, Opt};
@@ -13,24 +15,35 @@ use tracing_subscriber::fmt::format::FmtSpan;
 async fn main() {
     let opt = Opt::parse();
     let command = opt.command.unwrap_or(Command::Compile(opt.compile));
+
+    let current_working_directory = std::env::current_dir()
+        .expect("Expected current working to exist")
+        .to_str()
+        .expect("Expected current working directory to be able to be stringified.")
+        .intern()
+        .into();
+
     match command {
         Command::Compile(compile_command) => {
-            start_compiler(compile_command).await;
+            start_compiler(compile_command, current_working_directory).await;
         }
         Command::Lsp(lsp_command) => {
-            start_language_server(lsp_command).await;
+            start_language_server(lsp_command, current_working_directory).await;
         }
     }
 }
 
-async fn start_compiler(compile_command: CompileCommand) {
+async fn start_compiler(
+    compile_command: CompileCommand,
+    current_working_directory: CurrentWorkingDirectory,
+) {
     configure_logger(compile_command.log_level);
     let config_location = compile_command
         .config
         .unwrap_or("./isograph.config.json".into());
 
     if compile_command.watch {
-        match handle_watch_command(config_location).await {
+        match handle_watch_command(config_location, current_working_directory).await {
             Ok(res) => match res {
                 Ok(_) => {
                     info!("{}", "Successfully watched. Exiting.\n")
@@ -45,16 +58,20 @@ async fn start_compiler(compile_command: CompileCommand) {
                 std::process::exit(1);
             }
         };
-    } else if compile_and_print(config_location).is_err() {
+    } else if compile_and_print(config_location, current_working_directory).is_err() {
         std::process::exit(1);
     }
 }
 
-async fn start_language_server(lsp_command: LspCommand) {
+async fn start_language_server(
+    lsp_command: LspCommand,
+    current_working_directory: CurrentWorkingDirectory,
+) {
     let config = create_config(
         lsp_command
             .config
             .unwrap_or("./isograph.config.json".into()),
+        current_working_directory,
     );
     info!("Starting language server");
     if let Err(_e) = isograph_lsp::start_language_server(config).await {

crates/isograph_compiler/src/batch_compile.rs

@@ -2,7 +2,7 @@ use std::{path::PathBuf, str::Utf8Error};
 
 use crate::{with_duration::WithDuration, write_artifacts::GenerateArtifactsError};
 use colored::Colorize;
-use common_lang_types::WithLocation;
+use common_lang_types::{CurrentWorkingDirectory, WithLocation};
 use graphql_schema_parser::SchemaParseError;
 use isograph_lang_parser::IsographLiteralParseError;
 use isograph_schema::{ProcessClientFieldDeclarationError, ValidateSchemaError};
@@ -18,10 +18,13 @@ pub struct CompilationStats {
     pub total_artifacts_written: usize,
 }
 
-pub fn compile_and_print(config_location: PathBuf) -> Result<(), BatchCompileError> {
+pub fn compile_and_print(
+    config_location: PathBuf,
+    current_working_directory: CurrentWorkingDirectory,
+) -> Result<(), BatchCompileError> {
     info!("{}", "Starting to compile.".cyan());
     print_result(WithDuration::new(|| {
-        CompilerState::new(config_location).batch_compile()
+        CompilerState::new(config_location, current_working_directory).batch_compile()
     }))
 }
 

crates/isograph_compiler/src/compiler_state.rs

@@ -1,5 +1,6 @@
 use std::path::PathBuf;
 
+use common_lang_types::CurrentWorkingDirectory;
 use graphql_artifact_generation::get_artifact_path_and_content;
 use isograph_config::{create_config, CompilerConfig, GenerateFileExtensionsOption};
 use isograph_schema::{Schema, UnvalidatedSchema};
@@ -17,9 +18,12 @@ pub struct CompilerState {
 }
 
 impl CompilerState {
-    pub fn new(config_location: PathBuf) -> Self {
+    pub fn new(
+        config_location: PathBuf,
+        current_working_directory: CurrentWorkingDirectory,
+    ) -> Self {
         Self {
-            config: create_config(config_location),
+            config: create_config(config_location, current_working_directory),
             source_files: None,
         }
     }
@@ -165,11 +169,12 @@ pub fn validate_and_create_artifacts_from_source_files(
     let artifacts = get_artifact_path_and_content(
         &validated_schema,
         &config.project_root,
-        &config.artifact_directory,
+        &config.artifact_directory.absolute_path,
         file_extensions,
         no_babel_transform,
     );
 
-    let total_artifacts_written = write_artifacts_to_disk(artifacts, &config.artifact_directory)?;
+    let total_artifacts_written =
+        write_artifacts_to_disk(artifacts, &config.artifact_directory.absolute_path)?;
     Ok(total_artifacts_written)
 }