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

Add support for peer deps #1072

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

jakebailey
Copy link
Member

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.

@jakebailey
Copy link
Member Author

jakebailey commented Sep 30, 2024

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 @types/react because the user will have already installed it and can control the version. Anything more than that increases the annoyance levels for those with package managers which do not auto install peers.

I am not sure how we can possibly enforce that except via human interaction. Perhaps we should make mergebot flag any PR which modifies peerDeps as needing a maintainer review?

@sandersn
Copy link
Member

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.)

@jakebailey
Copy link
Member Author

Added a mergebot complaint which should put Check Config on those PRs, though I don't have a PR to test it on quite yet. Will make one at the risk of annoying someone...

Copy link
Member

@sandersn sandersn left a 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.

@jakebailey
Copy link
Member Author

I went ahead and added proper tests for the disallowed dep errors.

@jakebailey
Copy link
Member Author

@zkochan Do you have any thoughts on whether or not @types packages gaining peer deps will cause a problem for pnpm? I'm not sure what special casing pnpm does for types packages at the moment, but having these is likely to improve resolution situations with things like react components.

@zkochan
Copy link

zkochan commented Oct 15, 2024

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.

@jakebailey
Copy link
Member Author

jakebailey commented Oct 15, 2024

My expectation is that basically all @types/react-* ish packages will end up with peer deps on @types/react, and then some other examples similar to that. The intent is that types packages don't use peer deps unless the package they're describing have peer deps (or, should have declared something as a peer dep).

Hopefully the result is not too much more memory, but this change may be required for correctness ☹️

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.

Allow peer dependencies with stricter version ranges
3 participants