-
Notifications
You must be signed in to change notification settings - Fork 216
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
Add support for peer deps #1072
Conversation
It's worth nothing that peer deps are intended to be used in cases where the user is already going to install that peer dep. E.g. publishing a react component, where you peer dep on I am not sure how we can possibly enforce that except via human interaction. Perhaps we should make |
I think maintainer review is a good first state for any confusing systemic change. We can remove the requirement if it's onerous (but I doubt it'll be common, so I bet it's not a problem.) |
Added a mergebot complaint which should put |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far. But I don't have a way to tell from a review whether it's a complete change. I guess the mergebot snapshot test will help with that.
I went ahead and added proper tests for the disallowed dep errors. |
@zkochan Do you have any thoughts on whether or not |
pnpm struggles a lot with peer dependencies. The issue is that peer dependencies are used too frequently nowadays. To make things worth peer dependencies have peer dependencies of their own and some peer dependencies even form a circular connection. I am not sure how other package managers solve these issues. Maybe it is less of an issue in a hoisted node_modules. pnpm, however, needs to consciously resolve all the peer dependency combinations and because of the above complexities it becomes a very expensive operation. I've spent many hundreds of hours optimizing the peer dependencies resolution part of pnpm. So, if this change will result in even more peer dependencies, pnpm install will use more memory during peer dependencies resolution. Hopefully it won't cause out-of-memory errors. Maybe if we could kind of group the type peer with the package peer that it is related to and resolve them as one entity, then it wouldn't make peers resolution a harder task. |
My expectation is that basically all Hopefully the result is not too much more memory, but this change may be required for correctness |
Fixes #433
We've hit multiple cases now where peer deps would have really been beneficial, and now with the new (well, not new anymore) monorepo layout, we can handle these without much code change.
Likely needs more tests, though there really isn't much here besides plumbing.