Skip to content

Commit

Permalink
feature(turborepo): fancy package.json errors (#8299)
Browse files Browse the repository at this point in the history
### Description

More fancy errors! Since we now have span info for package.json files,
we can produce errors pointing to locations in package.json.

### Testing Instructions

<!--
  Give a quick description of steps to test your changes.
-->
  • Loading branch information
NicholasLYang authored Jul 19, 2024
1 parent b51e9e4 commit 5a68e3a
Show file tree
Hide file tree
Showing 16 changed files with 212 additions and 63 deletions.
1 change: 0 additions & 1 deletion crates/turborepo-errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,6 @@ impl<T> Deref for Spanned<T> {
&self.value
}
}

pub trait WithMetadata {
fn add_text(&mut self, text: Arc<str>);
fn add_path(&mut self, path: Arc<str>);
Expand Down
1 change: 1 addition & 0 deletions crates/turborepo-lib/src/cli/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ pub enum Error {
#[error(transparent)]
ChromeTracing(#[from] crate::tracing::Error),
#[error(transparent)]
#[diagnostic(transparent)]
BuildPackageGraph(#[from] package_graph::builder::Error),
#[error(transparent)]
Rewrite(#[from] RewriteError),
Expand Down
7 changes: 5 additions & 2 deletions crates/turborepo-lib/src/engine/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -586,8 +586,11 @@ mod test {
let scripts = if had_build {
BTreeMap::from_iter(
[
("build".to_string(), "echo built!".to_string()),
("dev".to_string(), "echo running dev!".to_string()),
("build".to_string(), Spanned::new("echo built!".to_string())),
(
"dev".to_string(),
Spanned::new("echo running dev!".to_string()),
),
]
.into_iter(),
)
Expand Down
2 changes: 2 additions & 0 deletions crates/turborepo-lib/src/run/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ pub enum Error {
#[diagnostic(transparent)]
PackageJson(#[from] turborepo_repository::package_json::Error),
#[error(transparent)]
#[diagnostic(transparent)]
PackageManager(#[from] turborepo_repository::package_manager::Error),
#[error(transparent)]
#[diagnostic(transparent)]
Expand All @@ -49,6 +50,7 @@ pub enum Error {
#[error(transparent)]
TaskHash(#[from] task_hash::Error),
#[error(transparent)]
#[diagnostic(transparent)]
Visitor(#[from] task_graph::VisitorError),
#[error("error registering signal handler: {0}")]
SignalHandler(std::io::Error),
Expand Down
1 change: 1 addition & 0 deletions crates/turborepo-lib/src/run/summary/task_factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ impl<'a> TaskSummaryFactory<'a> {
.package_json
.scripts
.get(task_id.task())
.map(|script| script.as_inner())
.cloned()
.unwrap_or_else(|| "<NONEXISTENT>".to_string());

Expand Down
15 changes: 13 additions & 2 deletions crates/turborepo-lib/src/task_graph/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use console::{Style, StyledObject};
use either::Either;
use futures::{stream::FuturesUnordered, StreamExt};
use itertools::Itertools;
use miette::{Diagnostic, NamedSource, SourceSpan};
use regex::Regex;
use tokio::sync::{mpsc, oneshot};
use tracing::{debug, error, Instrument, Span};
Expand Down Expand Up @@ -67,7 +68,7 @@ pub struct Visitor<'a> {
is_watch: bool,
}

#[derive(Debug, thiserror::Error)]
#[derive(Debug, thiserror::Error, Diagnostic)]
pub enum Error {
#[error("cannot find package {package_name} for task {task_id}")]
MissingPackage {
Expand All @@ -77,7 +78,14 @@ pub enum Error {
#[error(
"root task {task_name} ({command}) looks like it invokes turbo and might cause a loop"
)]
RecursiveTurbo { task_name: String, command: String },
RecursiveTurbo {
task_name: String,
command: String,
#[label("task found here")]
span: Option<SourceSpan>,
#[source_code]
text: NamedSource,
},
#[error("Could not find definition for task")]
MissingDefinition,
#[error("error while executing engine: {0}")]
Expand Down Expand Up @@ -186,9 +194,12 @@ impl<'a> Visitor<'a> {
match command {
Some(cmd) if info.package() == ROOT_PKG_NAME && turbo_regex().is_match(&cmd) => {
package_task_event.track_error(TrackedErrors::RecursiveError);
let (span, text) = cmd.span_and_text("package.json");
return Err(Error::RecursiveTurbo {
task_name: info.to_string(),
command: cmd.to_string(),
span,
text,
});
}
_ => (),
Expand Down
4 changes: 2 additions & 2 deletions crates/turborepo-lib/src/turbo_json/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -794,7 +794,7 @@ mod tests {
#[test_case(
None,
PackageJson {
scripts: [("build".to_string(), "echo build".to_string())].into_iter().collect(),
scripts: [("build".to_string(), Spanned::new("echo build".to_string()))].into_iter().collect(),
..PackageJson::default()
},
TurboJson {
Expand All @@ -818,7 +818,7 @@ mod tests {
}
}"#),
PackageJson {
scripts: [("test".to_string(), "echo test".to_string())].into_iter().collect(),
scripts: [("test".to_string(), Spanned::new("echo test".to_string()))].into_iter().collect(),
..PackageJson::default()
},
TurboJson {
Expand Down
13 changes: 4 additions & 9 deletions crates/turborepo-repository/src/package_graph/builder.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
use std::{
backtrace::Backtrace,
collections::{BTreeMap, HashMap, HashSet},
};
use std::collections::{BTreeMap, HashMap, HashSet};

use miette::Diagnostic;
use petgraph::graph::{Graph, NodeIndex};
Expand Down Expand Up @@ -36,11 +33,9 @@ pub struct PackageGraphBuilder<'a, T> {

#[derive(Debug, Diagnostic, thiserror::Error)]
pub enum Error {
#[error("could not resolve workspaces: {0}")]
PackageManager(
#[from] crate::package_manager::Error,
#[backtrace] Backtrace,
),
#[error("could not resolve workspaces")]
#[diagnostic(transparent)]
PackageManager(#[from] crate::package_manager::Error),
#[error(
"Failed to add workspace \"{name}\" from \"{path}\", it already exists at \
\"{existing_path}\""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ impl<'a> DependencyVersion<'a> {
_ => {
// If we got this far, then we need to check the workspace package version to
// see it satisfies the dependencies range to determin whether
// or not its an internal or external dependency.
// or not it's an internal or external dependency.
let constraint = node_semver::Range::parse(self.version);
let version = node_semver::Version::parse(package_version);

Expand Down
48 changes: 37 additions & 11 deletions crates/turborepo-repository/src/package_json.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
use std::collections::{BTreeMap, HashMap};
use std::{
collections::{BTreeMap, HashMap},
sync::Arc,
};

use anyhow::Result;
use biome_deserialize::{json::deserialize_from_json_str, Text};
Expand All @@ -8,7 +11,7 @@ use biome_json_parser::JsonParserOptions;
use miette::Diagnostic;
use serde::Serialize;
use turbopath::{AbsoluteSystemPath, RelativeUnixPathBuf};
use turborepo_errors::ParseDiagnostic;
use turborepo_errors::{ParseDiagnostic, Spanned, WithMetadata};
use turborepo_unescape::UnescapedString;

#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize)]
Expand All @@ -19,7 +22,7 @@ pub struct PackageJson {
#[serde(skip_serializing_if = "Option::is_none")]
pub version: Option<String>,
#[serde(skip_serializing_if = "Option::is_none")]
pub package_manager: Option<String>,
pub package_manager: Option<Spanned<String>>,
#[serde(skip_serializing_if = "Option::is_none")]
pub dependencies: Option<BTreeMap<String, String>>,
#[serde(skip_serializing_if = "Option::is_none")]
Expand All @@ -29,7 +32,7 @@ pub struct PackageJson {
#[serde(skip_serializing_if = "Option::is_none")]
pub peer_dependencies: Option<BTreeMap<String, String>>,
#[serde(default, skip_serializing_if = "BTreeMap::is_empty")]
pub scripts: BTreeMap<String, String>,
pub scripts: BTreeMap<String, Spanned<String>>,
#[serde(skip_serializing_if = "Option::is_none")]
pub resolutions: Option<BTreeMap<String, String>>,
#[serde(skip_serializing_if = "Option::is_none")]
Expand All @@ -53,12 +56,12 @@ pub struct PnpmConfig {
pub struct RawPackageJson {
pub name: Option<UnescapedString>,
pub version: Option<UnescapedString>,
pub package_manager: Option<UnescapedString>,
pub package_manager: Option<Spanned<UnescapedString>>,
pub dependencies: Option<BTreeMap<String, UnescapedString>>,
pub dev_dependencies: Option<BTreeMap<String, UnescapedString>>,
pub optional_dependencies: Option<BTreeMap<String, UnescapedString>>,
pub peer_dependencies: Option<BTreeMap<String, UnescapedString>>,
pub scripts: BTreeMap<String, UnescapedString>,
pub scripts: BTreeMap<String, Spanned<UnescapedString>>,
pub resolutions: Option<BTreeMap<String, UnescapedString>>,
pub pnpm: Option<RawPnpmConfig>,
// Unstructured fields kept for round trip capabilities
Expand All @@ -85,12 +88,32 @@ pub enum Error {
Parse(#[related] Vec<ParseDiagnostic>),
}

impl WithMetadata for RawPackageJson {
fn add_text(&mut self, text: Arc<str>) {
if let Some(ref mut package_manager) = self.package_manager {
package_manager.add_text(text.clone());
}
self.scripts
.iter_mut()
.for_each(|(_, v)| v.add_text(text.clone()));
}

fn add_path(&mut self, path: Arc<str>) {
if let Some(ref mut package_manager) = self.package_manager {
package_manager.add_path(path.clone());
}
self.scripts
.iter_mut()
.for_each(|(_, v)| v.add_path(path.clone()));
}
}

impl From<RawPackageJson> for PackageJson {
fn from(raw: RawPackageJson) -> Self {
Self {
name: raw.name.map(|s| s.into()),
version: raw.version.map(|s| s.into()),
package_manager: raw.package_manager.map(|s| s.into()),
package_manager: raw.package_manager.map(|s| s.map(|s| s.into())),
dependencies: raw
.dependencies
.map(|m| m.into_iter().map(|(k, v)| (k, v.into())).collect()),
Expand All @@ -106,7 +129,7 @@ impl From<RawPackageJson> for PackageJson {
scripts: raw
.scripts
.into_iter()
.map(|(k, v)| (k, v.into()))
.map(|(k, v)| (k, v.map(|v| v.into())))
.collect(),
resolutions: raw
.resolutions
Expand Down Expand Up @@ -158,9 +181,12 @@ impl PackageJson {
}

// We expect a result if there are no errors
Ok(result
.expect("no parse errors produced but no result")
.into())
let mut package_json = result.expect("no parse errors produced but no result");

package_json.add_path(path.into());
package_json.add_text(contents.into());

Ok(package_json.into())
}

// Utility method for easy construction of package.json during testing
Expand Down
Loading

0 comments on commit 5a68e3a

Please sign in to comment.