Skip to content

Commit

Permalink
refactor(turborepo): change the ui config to use an enum (#8919)
Browse files Browse the repository at this point in the history
### Description

Right now we internally represent the `ui` config as a boolean. This
isn't very extensible, so I refactored it to be an enum instead

### Testing Instructions

<!--
  Give a quick description of steps to test your changes.
-->
  • Loading branch information
NicholasLYang authored Aug 2, 2024
1 parent fefcf5a commit 3d90b3d
Show file tree
Hide file tree
Showing 12 changed files with 66 additions and 57 deletions.
4 changes: 2 additions & 2 deletions crates/turborepo-lib/src/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use crate::{
run::watch::WatchClient,
shim::TurboState,
tracing::TurboSubscriber,
turbo_json::UI as ConfigUI,
turbo_json::UIMode,
};

mod error;
Expand Down Expand Up @@ -185,7 +185,7 @@ pub struct Args {
pub heap: Option<String>,
/// Specify whether to use the streaming UI or TUI
#[clap(long, global = true, value_enum)]
pub ui: Option<ConfigUI>,
pub ui: Option<UIMode>,
/// Override the login endpoint
#[clap(long, global = true, value_parser)]
pub login: Option<String>,
Expand Down
7 changes: 2 additions & 5 deletions crates/turborepo-lib/src/commands/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,7 @@ use turborepo_repository::{
package_graph::PackageGraph, package_json::PackageJson, package_manager::PackageManager,
};

use crate::{
cli::{self, EnvMode},
commands::CommandBase,
};
use crate::{cli, cli::EnvMode, commands::CommandBase, turbo_json::UIMode};

#[derive(Debug, Serialize)]
#[serde(rename_all = "camelCase")]
Expand All @@ -21,7 +18,7 @@ struct ConfigOutput<'a> {
upload_timeout: u64,
enabled: bool,
spaces_id: Option<&'a str>,
ui: bool,
ui: UIMode,
package_manager: PackageManager,
daemon: Option<bool>,
env_mode: EnvMode,
Expand Down
5 changes: 3 additions & 2 deletions crates/turborepo-lib/src/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use turborepo_ui::UI;
use crate::{
cli::Command,
config::{ConfigurationOptions, Error as ConfigError, TurborepoConfigBuilder},
turbo_json::UIMode,
Args,
};

Expand Down Expand Up @@ -70,10 +71,10 @@ impl CommandBase {
.with_token(self.args.token.clone())
.with_timeout(self.args.remote_cache_timeout)
.with_preflight(self.args.preflight.then_some(true))
.with_ui(self.args.ui.map(|ui| ui.use_tui()).or_else(|| {
.with_ui(self.args.ui.or_else(|| {
self.args.execution_args.as_ref().and_then(|args| {
if !args.log_order.compatible_with_tui() {
Some(false)
Some(UIMode::Stream)
} else {
// If the argument is compatible with the TUI this does not mean we should
// override other configs
Expand Down
26 changes: 16 additions & 10 deletions crates/turborepo-lib/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use turborepo_auth::{TURBO_TOKEN_DIR, TURBO_TOKEN_FILE, VERCEL_TOKEN_DIR, VERCEL
use turborepo_dirs::{config_dir, vercel_config_dir};
use turborepo_errors::TURBO_SITE;

pub use crate::turbo_json::RawTurboJson;
pub use crate::turbo_json::{RawTurboJson, UIMode};
use crate::{cli::EnvMode, commands::CommandBase, turbo_json};

#[derive(Debug, Error, Diagnostic)]
Expand Down Expand Up @@ -207,7 +207,7 @@ pub struct ConfigurationOptions {
pub(crate) enabled: Option<bool>,
pub(crate) spaces_id: Option<String>,
#[serde(rename = "ui")]
pub(crate) ui: Option<bool>,
pub(crate) ui: Option<UIMode>,
#[serde(rename = "dangerouslyDisablePackageManagerCheck")]
pub(crate) allow_no_package_manager: Option<bool>,
pub(crate) daemon: Option<bool>,
Expand Down Expand Up @@ -276,8 +276,12 @@ impl ConfigurationOptions {
self.spaces_id.as_deref()
}

pub fn ui(&self) -> bool {
self.ui.unwrap_or(false) && atty::is(atty::Stream::Stdout)
pub fn ui(&self) -> UIMode {
if atty::is(atty::Stream::Stdout) {
return UIMode::Stream;
}

self.ui.unwrap_or(UIMode::Stream)
}

pub fn allow_no_package_manager(&self) -> bool {
Expand Down Expand Up @@ -323,7 +327,7 @@ impl ResolvedConfigurationOptions for RawTurboJson {
.experimental_spaces
.and_then(|spaces| spaces.id)
.map(|spaces_id| spaces_id.into());
opts.ui = self.ui.map(|ui| ui.use_tui());
opts.ui = self.ui;
opts.allow_no_package_manager = self.allow_no_package_manager;
opts.daemon = self.daemon.map(|daemon| *daemon.as_inner());
opts.env_mode = self.env_mode;
Expand Down Expand Up @@ -449,7 +453,8 @@ fn get_env_var_config(
let ui = output_map
.get("ui")
.map(|s| s.as_str())
.and_then(truth_env_var);
.and_then(truth_env_var)
.map(|ui| if ui { UIMode::Tui } else { UIMode::Stream });

let allow_no_package_manager = output_map
.get("allow_no_package_manager")
Expand Down Expand Up @@ -535,7 +540,7 @@ fn get_override_env_var_config(
.and_then(|value| {
// If either of these are truthy, then we disable the TUI
if value == "true" || value == "1" {
Some(false)
Some(UIMode::Stream)
} else {
None
}
Expand Down Expand Up @@ -692,7 +697,7 @@ impl TurborepoConfigBuilder {
create_builder!(with_enabled, enabled, Option<bool>);
create_builder!(with_preflight, preflight, Option<bool>);
create_builder!(with_timeout, timeout, Option<u64>);
create_builder!(with_ui, ui, Option<bool>);
create_builder!(with_ui, ui, Option<UIMode>);
create_builder!(
with_allow_no_package_manager,
allow_no_package_manager,
Expand Down Expand Up @@ -809,6 +814,7 @@ mod test {
get_env_var_config, get_override_env_var_config, ConfigurationOptions,
TurborepoConfigBuilder, DEFAULT_API_URL, DEFAULT_LOGIN_URL, DEFAULT_TIMEOUT,
},
turbo_json::UIMode,
};

#[test]
Expand Down Expand Up @@ -864,7 +870,7 @@ mod test {
assert_eq!(turbo_teamid, config.team_id.unwrap());
assert_eq!(turbo_token, config.token.unwrap());
assert_eq!(turbo_remote_cache_timeout, config.timeout.unwrap());
assert_eq!(Some(true), config.ui);
assert_eq!(Some(UIMode::Tui), config.ui);
assert_eq!(Some(true), config.allow_no_package_manager);
assert_eq!(Some(true), config.daemon);
assert_eq!(Some(EnvMode::Strict), config.env_mode);
Expand Down Expand Up @@ -915,7 +921,7 @@ mod test {
let config = get_override_env_var_config(&env).unwrap();
assert_eq!(vercel_artifacts_token, config.token.unwrap());
assert_eq!(vercel_artifacts_owner, config.team_id.unwrap());
assert_eq!(Some(false), config.ui);
assert_eq!(Some(UIMode::Stream), config.ui);
}

#[test]
Expand Down
22 changes: 13 additions & 9 deletions crates/turborepo-lib/src/engine/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use thiserror::Error;
use turborepo_errors::Spanned;
use turborepo_repository::package_graph::{PackageGraph, PackageName};

use crate::{run::task_id::TaskId, task_graph::TaskDefinition};
use crate::{run::task_id::TaskId, task_graph::TaskDefinition, turbo_json::UIMode};

#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
pub enum TaskNode {
Expand Down Expand Up @@ -383,7 +383,7 @@ impl Engine<Built> {
&self,
package_graph: &PackageGraph,
concurrency: u32,
experimental_ui: bool,
ui_mode: UIMode,
) -> Result<(), Vec<ValidateError>> {
// TODO(olszewski) once this is hooked up to a real run, we should
// see if using rayon to parallelize would provide a speedup
Expand Down Expand Up @@ -480,7 +480,7 @@ impl Engine<Built> {
})
}

validation_errors.extend(self.validate_interactive(experimental_ui));
validation_errors.extend(self.validate_interactive(ui_mode));

match validation_errors.is_empty() {
true => Ok(()),
Expand All @@ -489,10 +489,10 @@ impl Engine<Built> {
}

// Validates that UI is setup if any interactive tasks will be executed
fn validate_interactive(&self, experimental_ui: bool) -> Vec<ValidateError> {
fn validate_interactive(&self, ui_mode: UIMode) -> Vec<ValidateError> {
// If experimental_ui is being used, then we don't need check for interactive
// tasks
if experimental_ui {
if matches!(ui_mode, UIMode::Tui) {
return Vec::new();
}
self.task_definitions
Expand Down Expand Up @@ -666,17 +666,21 @@ mod test {
let graph = graph_builder.build().await.unwrap();

// if our limit is less than, it should fail
engine.validate(&graph, 1, false).expect_err("not enough");
engine
.validate(&graph, 1, UIMode::Stream)
.expect_err("not enough");

// if our limit is less than, it should fail
engine.validate(&graph, 2, false).expect_err("not enough");
engine
.validate(&graph, 2, UIMode::Stream)
.expect_err("not enough");

// we have two persistent tasks, and a slot for all other tasks, so this should
// pass
engine.validate(&graph, 3, false).expect("ok");
engine.validate(&graph, 3, UIMode::Stream).expect("ok");

// if our limit is greater, then it should pass
engine.validate(&graph, 4, false).expect("ok");
engine.validate(&graph, 4, UIMode::Stream).expect("ok");
}

#[tokio::test]
Expand Down
18 changes: 7 additions & 11 deletions crates/turborepo-lib/src/run/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ use crate::{
run::{scope, task_access::TaskAccess, task_id::TaskName, Error, Run, RunCache},
shim::TurboState,
signal::{SignalHandler, SignalSubscriber},
turbo_json::TurboJson,
turbo_json::{TurboJson, UIMode},
DaemonConnector,
};

Expand All @@ -56,7 +56,7 @@ pub struct RunBuilder {
repo_root: AbsoluteSystemPathBuf,
ui: UI,
version: &'static str,
experimental_ui: bool,
ui_mode: UIMode,
api_client: APIClient,
analytics_sender: Option<AnalyticsSender>,
// In watch mode, we can have a changed package that we want to serve as an entrypoint.
Expand All @@ -77,13 +77,13 @@ impl RunBuilder {
let allow_missing_package_manager = config.allow_no_package_manager();

let version = base.version();
let experimental_ui = config.ui();
let ui_mode = config.ui();
let processes = ProcessManager::new(
// We currently only use a pty if the following are met:
// - we're attached to a tty
atty::is(atty::Stream::Stdout) &&
// - if we're on windows, we're using the UI
(!cfg!(windows) || experimental_ui),
(!cfg!(windows) || matches!(ui_mode, UIMode::Tui)),
);
let CommandBase { repo_root, ui, .. } = base;

Expand All @@ -94,7 +94,7 @@ impl RunBuilder {
repo_root,
ui,
version,
experimental_ui,
ui_mode,
api_auth,
analytics_sender: None,
entrypoint_packages: None,
Expand Down Expand Up @@ -384,7 +384,7 @@ impl RunBuilder {
Ok(Run {
version: self.version,
ui: self.ui,
experimental_ui: self.experimental_ui,
ui_mode: self.ui_mode,
start_at,
processes: self.processes,
run_telemetry,
Expand Down Expand Up @@ -439,11 +439,7 @@ impl RunBuilder {

if !self.opts.run_opts.parallel {
engine
.validate(
pkg_dep_graph,
self.opts.run_opts.concurrency,
self.experimental_ui,
)
.validate(pkg_dep_graph, self.opts.run_opts.concurrency, self.ui_mode)
.map_err(Error::EngineValidation)?;
}

Expand Down
10 changes: 5 additions & 5 deletions crates/turborepo-lib/src/run/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ use crate::{
signal::SignalHandler,
task_graph::Visitor,
task_hash::{get_external_deps_hash, get_internal_deps_hash, PackageInputsHashes},
turbo_json::TurboJson,
turbo_json::{TurboJson, UIMode},
DaemonClient, DaemonConnector,
};

Expand All @@ -69,7 +69,7 @@ pub struct Run {
task_access: TaskAccess,
daemon: Option<DaemonClient<DaemonConnector>>,
should_print_prelude: bool,
experimental_ui: bool,
ui_mode: UIMode,
}

impl Run {
Expand Down Expand Up @@ -172,12 +172,12 @@ impl Run {
self.ui
}

pub fn has_experimental_ui(&self) -> bool {
self.experimental_ui
pub fn has_tui(&self) -> bool {
self.ui_mode.use_tui()
}

pub fn should_start_ui(&self) -> Result<bool, Error> {
Ok(self.experimental_ui
Ok(self.ui_mode.use_tui()
&& self.opts.run_opts.dry_run.is_none()
&& tui::terminal_big_enough()?)
}
Expand Down
2 changes: 1 addition & 1 deletion crates/turborepo-lib/src/run/watch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ impl WatchClient {

let mut events = client.package_changes().await?;

if !self.run.has_experimental_ui() {
if !self.run.has_tui() {
self.run.print_run_prelude();
}

Expand Down
20 changes: 10 additions & 10 deletions crates/turborepo-lib/src/turbo_json/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ pub struct RawTurboJson {
#[serde(skip_serializing_if = "Option::is_none")]
pub(crate) remote_cache: Option<RawRemoteCacheOptions>,
#[serde(skip_serializing_if = "Option::is_none", rename = "ui")]
pub ui: Option<UI>,
pub ui: Option<UIMode>,
#[serde(
skip_serializing_if = "Option::is_none",
rename = "dangerouslyDisablePackageManagerCheck"
Expand Down Expand Up @@ -169,22 +169,22 @@ impl DerefMut for Pipeline {
}
}

#[derive(Serialize, Debug, Copy, Clone, Deserializable, PartialEq, Eq, ValueEnum)]
#[derive(Serialize, Deserialize, Debug, Copy, Clone, Deserializable, PartialEq, Eq, ValueEnum)]
#[serde(rename_all = "camelCase")]
pub enum UI {
/// Use the TUI interface
pub enum UIMode {
/// Use the terminal user interface
Tui,
/// Use the standard output stream
Stream,
}

impl Default for UI {
impl Default for UIMode {
fn default() -> Self {
Self::Tui
}
}

impl UI {
impl UIMode {
pub fn use_tui(&self) -> bool {
matches!(self, Self::Tui)
}
Expand Down Expand Up @@ -749,7 +749,7 @@ mod tests {
use turborepo_repository::package_json::PackageJson;
use turborepo_unescape::UnescapedString;

use super::{Pipeline, RawTurboJson, Spanned, UI};
use super::{Pipeline, RawTurboJson, Spanned, UIMode};
use crate::{
cli::OutputLogsMode,
run::task_id::TaskName,
Expand Down Expand Up @@ -1079,10 +1079,10 @@ mod tests {
assert_eq!(actual, expected);
}

#[test_case(r#"{ "ui": "tui" }"#, Some(UI::Tui) ; "tui")]
#[test_case(r#"{ "ui": "stream" }"#, Some(UI::Stream) ; "stream")]
#[test_case(r#"{ "ui": "tui" }"#, Some(UIMode::Tui) ; "tui")]
#[test_case(r#"{ "ui": "stream" }"#, Some(UIMode::Stream) ; "stream")]
#[test_case(r#"{}"#, None ; "missing")]
fn test_ui(json: &str, expected: Option<UI>) {
fn test_ui(json: &str, expected: Option<UIMode>) {
let json = RawTurboJson::parse(json, AnchoredSystemPath::new("").unwrap()).unwrap();
assert_eq!(json.ui, expected);
}
Expand Down
2 changes: 1 addition & 1 deletion turborepo-tests/integration/tests/config.t
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ Run test run
"uploadTimeout": 60,
"enabled": true,
"spacesId": null,
"ui": false,
"ui": "stream",
"packageManager": "npm",
"daemon": null,
"envMode": "strict"
Expand Down
Loading

0 comments on commit 3d90b3d

Please sign in to comment.