-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(peer-deps): don't consider peer deps when constructing graph #813
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/vercel/turbo-site/4KpZc4kRp76XahD1ixrwvc6cQBnG |
I tried this first when attempting to fix my own build issues after discovering vercel#407. It wasn't enough to solve the build hangs with my project, but seems to have its own merrits. 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: vercel#407
0b759d1
to
2df44bb
Compare
@gsoltis is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
How can we progress this pr? |
@weyert @thebanjomatic I am also currently looking into the flaky test that seems to have popped up and is blocking a bunch of merges. |
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