fix(dep-graph): allow mix of internal and external deps with same name #802

thebanjomatic · 2022-03-01T22:09:05Z

This is my first time coding with golang, so please let me know if there is anything non-idiomatic or where things can be written better.

When turbo currently tries to build the dependency graph, it doesn't consider the specified
version/range of the dependency, it just assumes that if that package name exists in the
workspace it must be an internal dependency. Because of this, cycles are detected and
the process hangs.

This PR adds some additional logic to ensure that dependencies are only considered
internal if the local/workspace package's version matches the semver range specified for
the dependency.

Fixes: #796

vercel · 2022-03-01T22:09:10Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/vercel/turbo-site/CoqtuFcwmnRsm8ngoyTKxGdF8Axr
✅ Preview: https://turbo-site-git-fork-thebanjomatic-fix-graphcheck-version.vercel.sh

[Deployment for 6daf7f5 failed]

gsoltis

Two general comments:

The first change I think makes sense, with the caveat that we should not error out if we fail to parse a version, as I think that would be a regression
I think I'm on board with the second commit in principle, but I think it ought to be a separate PR. In the event that it needs to be reverted, it would be better if it were independent.

thebanjomatic · 2022-03-02T20:16:51Z

The first change I think makes sense, with the caveat that we should not error out if we fail to parse a version, as I think that would be a regression

To confirm, if we fail to parse either the semver range or the package.json version, we should fallback to an internal package reference in that case to keep existing behavior?

I'll work on splitting the other commit out to its own PR. Its a little annoying because it touches the same code, but I can just target them both at main for now and then deal with the merge conflict whenever one or the other gets merged.

gsoltis · 2022-03-02T20:30:44Z

To confirm, if we fail to parse either the semver range or the package.json version, we should fallback to an internal package reference in that case to keep existing behavior?

Correct.

thebanjomatic · 2022-03-02T22:47:38Z

cli/internal/context/context.go

-		if item, ok := c.PackageInfos[dependencyName]; ok {
-			internalDepsSet.Add(item.Name)
+	for dependencyName, version := range depMap {
+		if IsReferenceToLocalPackage(c.PackageInfos, dependencyName, version) {


I rolled the code that looks up from the c.PackageInfos map into the isInternal check since logically that is what it means for the package to not be found in the map. But I suppose this could also be:

if item, ok := c.PackageInfos[dependencyName]; ok && IsReferenceToLocalPackage(item, version) {

It's not the biggest deal, but I think I like your proposed alternative better. That would allow isReferenceToLocalPackage to be a more focused function.

gsoltis

This is coming along. As a general Go note: to mark a function or method as package-private, start the name with a lowercase letter. For example, isReferenceToLocalPackage. Test code is typically colocated in the same package as the code under test, so this doesn't impact the ability to right tests.

And on that topic, I think it would be helpful to have a few examples of internal and external references in a test of isReferenceToLocalPackage to illustrate how the feature is intended to work.

gsoltis · 2022-03-03T18:29:14Z

cli/internal/context/context.go

+		// for an exact match.
+		return true
+	} else if IsProtocolExternal(protocol) {
+		// Other protocols are assumed to be external references ("git:", "npm:", "file:" etc)


This comment implies that npm: is an external reference, however the implementation will return false if protocol == "npm".

gsoltis · 2022-03-03T18:40:10Z

cli/internal/context/context.go

-		if item, ok := c.PackageInfos[dependencyName]; ok {
-			internalDepsSet.Add(item.Name)
+	for dependencyName, version := range depMap {
+		if IsReferenceToLocalPackage(c.PackageInfos, dependencyName, version) {


It's not the biggest deal, but I think I like your proposed alternative better. That would allow isReferenceToLocalPackage to be a more focused function.

When turbo currently tries to build the dependency graph, it doesn't consider the specified version/range of the dependency, it just assumes that if that package name exists in the workspace it must be an internal dependency. Because of this the cycles are detected and the process hangs. This PR adds some additional logic to ensure that dependencies are only considered internal if the local/workspace package's version matches the semver range specified for the dependency. Fixes: vercel#796

thebanjomatic · 2022-03-03T20:24:19Z

@gsoltis thanks for the review and golang tips. I have refactored things so that IsReferenceToLocalPackage => isWorkspaceReference(packageVersion string, dependencyVersion).

Having it just take two strings as input does make it a lot easier to test. I think it should have good coverage of the branches.

gsoltis

Awesome!

Splitting this out of #802. I tried this first when attempting to fix my own build issues in that PR after discovering Issue #407 which suggested peerDependencies might be the problem. It wasn't enough to solve the build hangs with my project, but seems to have its own merits. If a package has a peer dependency but there is no corresponding devDependency, then it should be presumed that there are no build-time dependency on that package. For example, if there was code that would be executed by one of the scripts that tried to import from the peer package, this would generally speaking fail to run, because node would be unable to resolve the dependency unless another package was pulling it in and it got hoisted, etc. Workspace dependencies are kind of a special case in that they won't be hoisted and can be resolved by node, but if you were to ever move that package out of the monorepo then that same situation as above would fail to build. As a result, my impression is that for any use-case where peerDependencies need to be explicitly part of the dependency graph is actually just a missing dependency declaration bug where that package should actually be part of the dev deps. I have also changed the order in which dependencies, optionalDependencies, and devDependencies are selected to match the order that lerna uses for cases where the package shows up in multiple places. See: https://github.com/lerna/lerna/blob/a47fc294393a3e9507a8207a5a2f07648a524722/core/package-graph/index.js#L53-L58 The attached issues mentions making a flag to control this behavior, but given the reasoning above, I'm not sure if there is ever a situation where you would want to have a peer-dependency impact the dependency graph if its not also a regular or dev (or optional) dependency. Fixes: #407

thebanjomatic requested review from jaredpalmer, gsoltis and gaspar09 as code owners March 1, 2022 22:09

vercel bot deployed to Preview March 1, 2022 22:09 View deployment

gsoltis reviewed Mar 2, 2022

View reviewed changes

thebanjomatic force-pushed the fix/graph/check-version branch from 8b3a2d0 to 4136e11 Compare March 2, 2022 22:30

vercel bot deployed to Preview March 2, 2022 22:30 View deployment

thebanjomatic mentioned this pull request Mar 2, 2022

fix(peer-deps): don't consider peer deps when constructing graph #813

Merged

thebanjomatic changed the title ~~fix(dep-graph): avoid hangs from erroneous cycle detection~~ fix(dep-graph): allow mix of internal and external deps with same name Mar 2, 2022

thebanjomatic requested a review from gsoltis March 2, 2022 22:33

thebanjomatic force-pushed the fix/graph/check-version branch from 4136e11 to a851905 Compare March 2, 2022 22:43

vercel bot deployed to Preview March 2, 2022 22:43 View deployment

thebanjomatic commented Mar 2, 2022

View reviewed changes

vercel bot had a problem deploying to Preview March 3, 2022 14:06 Failure

gsoltis suggested changes Mar 3, 2022

View reviewed changes

thebanjomatic force-pushed the fix/graph/check-version branch from 7fb922b to d176f03 Compare March 3, 2022 20:20

vercel bot had a problem deploying to Preview March 3, 2022 20:20 Failure

thebanjomatic requested a review from gsoltis March 3, 2022 20:21

Merge branch 'main' into fix/graph/check-version

6daf7f5

vercel bot had a problem deploying to Preview March 3, 2022 22:00 Failure

gsoltis approved these changes Mar 3, 2022

View reviewed changes

kodiakhq bot merged commit 9886af2 into vercel:main Mar 3, 2022

thebanjomatic deleted the fix/graph/check-version branch March 3, 2022 23:12

gsoltis mentioned this pull request Mar 9, 2022

BREAKING REGRESSION: Turbo no longer recognizes and builds dependencies / devDependencies for ^build #847

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dep-graph): allow mix of internal and external deps with same name #802

fix(dep-graph): allow mix of internal and external deps with same name #802

thebanjomatic commented Mar 1, 2022 •

edited

Loading

vercel bot commented Mar 1, 2022 •

edited

Loading

gsoltis left a comment

thebanjomatic commented Mar 2, 2022

gsoltis commented Mar 2, 2022

thebanjomatic Mar 2, 2022

gsoltis Mar 3, 2022

gsoltis left a comment

gsoltis Mar 3, 2022

gsoltis Mar 3, 2022

thebanjomatic commented Mar 3, 2022

gsoltis left a comment

fix(dep-graph): allow mix of internal and external deps with same name #802

fix(dep-graph): allow mix of internal and external deps with same name #802

Conversation

thebanjomatic commented Mar 1, 2022 • edited Loading

vercel bot commented Mar 1, 2022 • edited Loading

gsoltis left a comment

Choose a reason for hiding this comment

thebanjomatic commented Mar 2, 2022

gsoltis commented Mar 2, 2022

thebanjomatic Mar 2, 2022

Choose a reason for hiding this comment

gsoltis Mar 3, 2022

Choose a reason for hiding this comment

gsoltis left a comment

Choose a reason for hiding this comment

gsoltis Mar 3, 2022

Choose a reason for hiding this comment

gsoltis Mar 3, 2022

Choose a reason for hiding this comment

thebanjomatic commented Mar 3, 2022

gsoltis left a comment

Choose a reason for hiding this comment

thebanjomatic commented Mar 1, 2022 •

edited

Loading

vercel bot commented Mar 1, 2022 •

edited

Loading