这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@ObliviousHarmony
Copy link
Contributor

Description

When using a global install of turbo there is no way to be sure it is compatible with the version of turbo required by a given project. There seems to be a standard for tools to support validating against the engines property in the package.json file. This pull request adds that validation to turbo commands.

Closes #1359.

Testing Instructions

  1. Add a "1.0.0" constraint to package.json in the engines.turbo property
  2. Run ./turbow.sh and note that it returns an error
  3. Change the package.json constraint to "^1.0"
  4. Run ./turbow.sh and note that it displays the help output
  5. Change the package.json constraint to "test"
  6. Run ./turbow.sh and note that it displays an error

@vercel
Copy link
Contributor

vercel bot commented Jun 9, 2022

@ObliviousHarmony is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

@nathanhammond
Copy link
Contributor

Hey @ObliviousHarmony! I think that this functionality is definitely something we should consider; we're looking into what operating as a global installation would look like and this is a good step along that path.

Things that this should account for:

  • We're not guaranteed to have a package.json in all of our possible futures so things that configure turbo should probably live in our own namespace, probably turbo.json.
  • version.go is explicitly intended to be nothing more than a version string; that check should live somewhere else.
  • We should scope this behavior to things which interact with the user's world. turbo run and turbo prune are likely to be tied to the current project; whereas turbo link and turbo login are unlikely to need precise versioning.

What do you think?

@ObliviousHarmony
Copy link
Contributor Author

Thanks for the feedback @nathanhammond!

we're looking into what operating as a global installation would look like

My first thought was that turbo from the global installation should proxy to the local one when it's present. However, I wasn't sure about the ambiguity that might introduce, so I decided on this approach instead. How would it decide what the "local" version is? Traverse through ./node_modules in each directory and parent directory until it finds one? It just feels like that might be too prone to strange issues.

We're not guaranteed to have a package.json in all of our possible futures so things that configure turbo should probably live in our own namespace, probably turbo.json.

Do you think supporting both might be ambiguous? Centralizing these kinds of constraints in package.json feels more aligned with Node projects. I can imagine possible futures where turbo supports other workspaces and projects in other languages, but, it seems like that would provide dependency and execution information separate from the existing package.json functionality.

version.go is explicitly intended to be nothing more than a version string; that check should live somewhere else.

Along the lines of the above, how would you feel about the following changes:

  • Add local_version_constraint.go to main package
    • Add checkCompatibility(version string) bool that does the same comparison that ValidateTurboEngineVersion() does.
    • Add IsGlobalVersionCompatible(config config.Config) bool that, for now, only checks fs.PackageJSON.

This would make it trivial to expand IsGlobalVersionCompatible() handling for other sources for the version constraint. I would argue against adding it to turbo.json just yet, since currently, package.json already contains all of the scripts and dependency information. It feels reasonable that it would also use engines.turbo over a turbo.json config key.

We should scope this behavior to things which interact with the user's world. turbo run and turbo prune are likely to be tied to the current project; whereas turbo link and turbo login are unlikely to need precise versioning.

This seems fair, however, I have a question first. Is there anything local to .turbo in a given project that might run into a compatibility issue? My concern is that the functionality of turbo and pnpm -- turbo may be radically different, and in that scenario, we probably don't want to accidentally break anything.

@nathanhammond
Copy link
Contributor

So, in terms of delegating to the project-local version, that might look something like this:
https://github.com/ember-cli/ember-cli/blob/master/bin/ember

Alternatively, it might not delegate to a project local version and just error. And how that interacts with a versioned daemon is also a fun problem to solve. These are future hypotheticals, we haven't decided what path we should to take. When this one comes around it'll be an RFC.


We're definitely not going to be JavaScript-only forever. If you look at our own repository we have a Go project in here. We currently wire it up via package.json, but you can see how that just delegates down to make in maybe a hint of things to come:

"scripts": {
"clean": "make clean",
"build": "make",
"test": "make test-go",
"e2e": "make e2e",
"publish": "make publish-all",
"format": "make fmt-go",
"lint": "make lint-go"
},

We have already begun the process of moving out of package.json (turbo.json used to just be the turbo key inside of package.json). You can imagine that, if we want to increase the market that we are able to address as a product, we can't be dependent upon a particular ecosystem's idioms and need to define our own.

We also have to account for things like teachability and transferability of knowledge. Integrating in one way with the JavaScript ecosystem and another way with (everybody else) means that we are creating additional impedance for people learning. turbo.json is ours, and we can always guarantee it will exist and function in the same way (until/unless we decide to change that fact). No other parts of the system have that guarantee as we don't control them.


The .turbo stuff will eventually be pulled out of the project directory. That is also there by historical accident. All things that are part of that are content-addressed and we carefully ensure version compatibility with that.

@ObliviousHarmony
Copy link
Contributor Author

We're definitely not going to be JavaScript-only forever. If you look at our own repository we have a Go project in here. We currently wire it up via package.json, but you can see how that just delegates down to make in maybe a hint of things to come

We are also operating a multi-language monorepo, so, I appreciate anything that makes working with that use-case easier. I'm kind of curious how you plan to handle dependency management in those cases though. As an aside, I've never worked with make before, and seeing the MakeFile makes (😄) me wish I had.

Taking a step away from this pull request for a moment though, I do want to stress that having a single source of truth for project names, scripts, and dependencies is nice. One of the things that created confusion when we used Nx was that each project had a configuration file that contained duplicate content that needed to be maintained in both files. We didn't gain anything from this difference and in a few cases, people added scripts to the wrong file and created problems in our tooling. All the Nx commands did was call scripts in pacakge.json files since we couldn't use their plugins due to their strong versioning opinions.

The integration with each project's package.json file is, to me, one of the great things about Turborepo. This cuts down on superfluous configuration, and in any possible future, it would be great to find a way to lean into that strength. With that said, however, perhaps having no scripts in package.json other than lifecycle hooks would be better. At least that way, nobody will accidentally run pnpm build and fail because turbo didn't build all of the dependencies first.

You can imagine that, if we want to increase the market that we are able to address as a product, we can't be dependent upon a particular ecosystem's idioms and need to define our own.

I agree with you. Decentralizing the configuration like this would only make it confusing for people. It's much better to keep Turborepo's configuration together.

I'll go ahead and make some changes to this pull request accordingly.

@nathanhammond
Copy link
Contributor

I'm kind of curious how you plan to handle dependency management in those cases though.

We are too! But we definitely have ideas. Right now the order of operations is pnpm install > turbo > package.json. That first step is kind of a bit weird and a significant source of confusion.

As an aside, I've never worked with make before

From an internal document: "People have been rebuilding make with a prettier interface for decades." The fundamental behavior of builds haven't changed since make was introduced 40-something years ago. They're just a DAG and since then we've just been exploring the best way to build the DAG, at what granularity, and how deeply we integrate that DAG with application concerns.

I do want to stress that having a single source of truth for project names, scripts, and dependencies is nice.

It's actually good to see this portion of our hypothesis panning out. From another internal document: "One of the most important aspects of Turborepo is that it does less, and yet still provides value." We're well aware that the seamlessness of the integration is an important part of the value proposition and will be very careful to avoid screwing that up. You should expect everything that currently works to still work, with opt-in to more complexity and (presumably) more reward.

perhaps having no scripts in package.json

Interesting proposal you have there. 😜


As to the actual implementation of this PR: I have no real concerns with your proposed approach. We need to have a conversation amongst the team about the priority of being able to stand alone before landing, but we would have that conversation to unblock this PR.

Aside: we should make an active decision about the other places we could choose to read this information from:

  • devDependencies
  • optionalDependencies
  • peerDependencies
  • engines

(I don't have an opinion at this time, just enumerating.)

@ObliviousHarmony
Copy link
Contributor Author

We are too! But we definitely have ideas. Right now the order of operations is pnpm install > turbo > package.json. That first step is kind of a bit weird and a significant source of confusion.

As a consumer, ideally, Turborepo would take the step of reading dependencies from relevant project files. Node projects from a package.json file, PHP projects from a composer.json file, Rust from a Cargo.toml file, Go from dependencies read from source files, etc.

The only additional feedback I would add here is that currently, defining implicit dependencies with a separate pipeline that is otherwise identical is a little strange.

The fundamental behavior of builds haven't changed since make was introduced 40-something years ago.

Do you ever feel like modern software engineering is just us trying to rediscover the practices and processes followed by electrical and mechanical engineers 50 years ago?

We're well aware that the seamlessness of the integration is an important part of the value proposition and will be very careful to avoid screwing that up.

👍🏼

Interesting proposal you have there. 😜

I'll argue that's more your proposal than mine, obviously. 😄 You weren't very ambiguous referencing the cli's package.json file.

I have no real concerns with your proposed approach.

That's good to hear, I'll go ahead with that proposed approach then.

we should make an active decision about the other places we could choose to read this information from

I thought about this initially but felt it inverted the relationship between turbo and the package manager. Turborepo is only a dependency of the monorepo's root package.json file because that provides convenience over installing it as a prerequisite. I don't see turbo as a command provided by the package manager so much as a replacement for the task execution functionality of the package manager. That's the line of thinking that led to opening #1254.

Combining that enhancement request and this pull request, the end result is that you would only ever use pnpm commands for dependency management and lifecycle hooks. With that context, I think it makes sense to only read from "engines.turbo". For what it's worth, if an incompatible version becomes a dependency, it would throw an error just running the binary from the dependency.

@ObliviousHarmony
Copy link
Contributor Author

@nathanhammond I've updated the pull request based on your feedback.

  • I put the compatibility check in the config package. It feels like it belongs here right now because this check would either be a facet of the RootPackageJSON or the TurboJSON. I recognize the Config might not be a good place for these references to live long-term, but, if that refactor ever happens, this can be moved then.
  • I kept the check as blocking for all commands. The question for me is what it should mean when a consumer explicitly says that they only want to support using a version of Turbo that is compatible with their project. The other thing is a maintenance concern. What's the likelihood someone might not add this check if a new command is added that should use it?

@nathanhammond
Copy link
Contributor

Adding this PR to our weekly internal team meeting agenda to unblock.

@nathanhammond
Copy link
Contributor

Okay, here's the team feedback:

  • Version is definitely going to be in turbo.json; that ended up unanimous among the team. A few months ago we had less-developed opinions, but seeing where we want to go and how we want to get there, that's going to make the most sense and lead to the least churn. We promise that it will eventually be a very sensible default.
  • We feel like we need to at least enumerate our plan for global installation (global shim, delegating to the in-project runner, communicating with multiple versions of the daemon all running on the same system) before landing. My job this week is to get the daemon stuff landed and then circle back to some of the versioning questions that introduces as we're definitely going to have multiple of our own processes to coordinate.

When I circle back around to that would it be okay for me to cherry-pick your commit into my PR?

@ObliviousHarmony
Copy link
Contributor Author

When I circle back around to that would it be okay for me to cherry-pick your commit into my PR?

Sure, there's not much to cherry-pick though 😄 Thanks for getting back to me @nathanhammond.

@nathanhammond
Copy link
Contributor

Hey! We've gotten global turbo sorted out now, and version is either "whatever is global" or delegates to the locally-installed version.

This now has to be implemented in Rust, so I'm going to close this PR. Look for a new PR soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Respect "engines" in package.json

2 participants