Skip to content
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

Merged
merged 4 commits into from
Mar 8, 2022

Conversation

thebanjomatic
Copy link
Contributor

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

@vercel
Copy link

vercel bot commented Mar 2, 2022

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/4KpZc4kRp76XahD1ixrwvc6cQBnG
✅ Preview: https://turbo-site-git-fork-thebanjomatic-fix-peer-deps.vercel.sh

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
@vercel
Copy link

vercel bot commented Mar 4, 2022

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

A member of the Team first needs to authorize it.

@weyert
Copy link
Contributor

weyert commented Mar 7, 2022

How can we progress this pr?

@gsoltis
Copy link
Contributor

gsoltis commented Mar 7, 2022

@weyert @thebanjomatic I am also currently looking into the flaky test that seems to have popped up and is blocking a bunch of merges.

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.

Add a flag to ignore local peer dependency declarations so that circular graph doesn't hang-up entire process
3 participants