feat: allow usage of turbo without turbo.json
#9149
Conversation
…ead of path based
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
8 Skipped Deployments
|
| @@ -650,41 +626,6 @@ mod test { | |||
| .unwrap() | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_turbo_json_loading() { | |||
There was a problem hiding this comment.
This test is moved to TurboJsonLoader unit tests
| /// Create a loader that will load turbo.json files throughout the workspace | ||
| pub fn workspace( | ||
| repo_root: AbsoluteSystemPathBuf, | ||
| packages: HashMap<PackageName, AbsoluteSystemPathBuf>, |
There was a problem hiding this comment.
This could be made more ergonomic by just taking &PackageGraph and keeping the extraction logic in this module. I avoided this to allow for testing loaders without constructing an entire package graph.
| /// Load a turbo.json for a given package | ||
| pub fn load(&self, package: &PackageName) -> Result<TurboJson, Error> { | ||
| pub fn load<'a>(&'a mut self, package: &PackageName) -> Result<&'a TurboJson, Error> { | ||
| if !self.cache.contains_key(package) { |
There was a problem hiding this comment.
I'm guessing the Entry API doesn't work here cause of the partial borrows?
| @@ -114,6 +144,7 @@ impl TurboJsonLoader { | |||
| ) | |||
| } | |||
| } | |||
| Strategy::Noop => Err(Error::NoTurboJSON), | |||
There was a problem hiding this comment.
I'm wondering if we should make the API into a Result<Option<TurboJson>, Error> to more explicitly force callers to handle the non-existent turbo.json case
There was a problem hiding this comment.
Only when building the engine do we view a non-existent turbo.json as a non-error. I'm not sure requiring the other callers to construct the NoTurboJSON error type manually is worth the better ergonomics in the engine builder.
| let task_name = TaskName::from(script.as_str()).into_root_task(); | ||
| turbo_json.tasks.insert( | ||
| task_name, | ||
| Spanned::new(RawTaskDefinition { |
There was a problem hiding this comment.
it might be worth keeping some span info here. We can take the script span from package.json and use it as the task definition location
There was a problem hiding this comment.
Having trouble getting spans for the script names as BTreeMap requires impl Ord for Spanned<T> which has issues since it contains Range which doesn't implement Ord.
I don't think this is worth the effort to plumb through. The only place this span would get used would be for validating no package task syntax and having that error point to a package.json isn't helpful when we're the ones generating turbo.json.
### Description In response to #9149 (comment) this PR avoids mutating `opts` when building `Run` we now set env mode at the task level. A majority of this PR is adding the ability to set env mode at the task level. ### Testing Instructions Updated integration test (update needed as task hash changed since env mode is now set at the task level instead of the global level).
chris-olszewski
deleted the
chrisolszewski/fdrtn-177-allow-running-turbo-without-turbojson
branch
Description
This PR adds the ability to use
turboin a monorepo that doesn't have aturbo.json. This feature is currently gated behind--experimental-allow-no-turbo-json/TURBO_ALLOW_NO_TURBO_JSON.A majority of the PR is refactoring the
EngineBuilderso that it no longer directly loadsTurboJsons, but delegates to aTurboJsonLoader. This allows us to use different strategies for resolvingTurboJsonloads depending on runtime options e.g. single package mode or task access.Reviewing this PR is best done by viewing each commit individually.
Testing Instructions
Unit testing for
turbo.jsonloading changes.Integration test for verifying the new loader is activated with the new env var/flag.