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: prevent from logging peer-dependencies that are met #2796

Closed
wants to merge 1 commit into from
Closed

fix: prevent from logging peer-dependencies that are met #2796

wants to merge 1 commit into from

Conversation

evenstensberg
Copy link

@evenstensberg evenstensberg commented Feb 27, 2017

Summary

Solves the problem where peerDependencies are warning even though they are met.

Test plan

Run yarn add react react-dom

Before:

skjermbilde 2017-02-27 kl 23 56 20

After:

skjermbilde 2017-02-27 kl 23 57 29

I don't really know if the fix is legit. I removed a line of code, it may be doing something else than it was in my context, but the error is from lib/package-resolver.js L370, where it removes the peerDependencies we expect to have ( later adding the peerDep? ) . May need further investigation, but from the second check in lib/package-linker , it returns undefined for the package, since it has been removed from this.patterns. My suggestion, if this PR is somewhat volatile, is to check the packages you've got from the upper-tree against the ones you have, and then remove dupes.

Related:

If there's anything you need for me, feel free to assign me for other stuff.

Even

@bestander
Copy link
Member

Looks like it is not that easy and broke some tests

@bestander bestander self-assigned this Feb 27, 2017
@evenstensberg
Copy link
Author

Yep. Assumed that! The bug might be more extensive than one might think. I ran yarn locally and it worked fine then, do you have an idea of what that might come from? The resolver is working as if should, but it may be that it gets the wrong information before resolving further layers upwards.

@bestander
Copy link
Member

Looks like it broke cases when packages get updated, i.e. patterns are replaced.
Your change seems logical so maybe there is some replace logic down the line that relies on this.patterns not being deleted yet.

@evenstensberg
Copy link
Author

I'll have a look at it tomorrow. It's doable to update packages as it is doing in multiple if loops in resolve packages I think

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.

None yet

2 participants