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

Respect peer dependencies #869

Merged
merged 7 commits into from
Apr 7, 2021
Merged

Respect peer dependencies #869

merged 7 commits into from
Apr 7, 2021

Conversation

korelstar
Copy link
Contributor

@korelstar korelstar commented Apr 1, 2021

The purpose of this PR is to fix #376.

@raineorshine Please have a look if this goes in the correct direction. Open questions are, e.g.:

  • should the check be optional, what is default?
  • which warning should be raised if detection does not work? (the tests are currently failing due to this warning)

I've exported the new getPeerDependencies for the purpose that other tools can use it too, e.g. https://github.com/th0r/npm-upgrade

Copy link
Owner

@raineorshine raineorshine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this PR is to fix #376.

@raineorshine Please have a look if this goes in the correct direction.

Thanks for your contribution!

  • should the check be optional, what is default?

It should be opt-in via the --peer option. Default ncu behavior is intentionally kept very minimal so it is easy to reason about and there are no surprises.

  • which warning should be raised if detection does not work? (the tests are currently failing due to this warning)

Can you explain more what you mean by "does not work"? Do you mean an upgrade breaks a peer dependency (expected), or do you mean the detection algorithm doesn't work in some cases (unexpected)? Are there other cases besides when packages have not yet been installed that you are thinking of?

Some other thoughts:

  • Should ncu ignore breaking peer dependencies completely when this option is provided, or should it suggest the latest version that is compatible? Not sure what would be expected/most useful.
  • Unit test required

Thanks!

lib/index.js Outdated Show resolved Hide resolved
@korelstar
Copy link
Contributor Author

It should be opt-in via the --peer option. Default ncu behavior is intentionally kept very minimal so it is easy to reason about and there are no surprises.

The --peer option can't be used, since there is already an attribute peer in the options variable which is true by default. It looks like it is filled by the --dep command line option. Therefore, I have chosen the option --checkPeer for now.

  • which warning should be raised if detection does not work? (the tests are currently failing due to this warning)

Can you explain more what you mean by "does not work"? Do you mean an upgrade breaks a peer dependency (expected), or do you mean the detection algorithm doesn't work in some cases (unexpected)? Are there other cases besides when packages have not yet been installed that you are thinking of?

In fact, I think it's only relevant when a package is not installed locally. An upgrade should never break a peer dependency, since the purpose of this PR is to check this (see next comment).

  • Should ncu ignore breaking peer dependencies completely when this option is provided, or should it suggest the latest version that is compatible? Not sure what would be expected/most useful.

My goal is that ncu suggests the latest version that is compatible to the peer dependencies. I think this is what a user would expect and this is what I've implemented. I don't think a user would expect that peer dependency are ignored completely.

  • Unit test required

I've added a test, now. The test uses a package ncu-test-peer (which has a peer dependency to ncu-test-return-version) for this purpose. We can transfer the ownership to you, if you want.

@korelstar korelstar requested a review from raineorshine April 5, 2021 14:43
@raineorshine
Copy link
Owner

It should be opt-in via the --peer option. Default ncu behavior is intentionally kept very minimal so it is easy to reason about and there are no surprises.

The --peer option can't be used, since there is already an attribute peer in the options variable which is true by default. It looks like it is filled by the --dep command line option. Therefore, I have chosen the option --checkPeer for now.

Perfect! I will go ahead and remove those options since they are legacy and internal only.

  • Should ncu ignore breaking peer dependencies completely when this option is provided, or should it suggest the latest version that is compatible? Not sure what would be expected/most useful.

My goal is that ncu suggests the latest version that is compatible to the peer dependencies. I think this is what a user would expect and this is what I've implemented. I don't think a user would expect that peer dependency are ignored completely.

That makes sense.

  • Unit test required

I've added a test, now. The test uses a package ncu-test-peer (which has a peer dependency to ncu-test-return-version) for this purpose. We can transfer the ownership to you, if you want.

Yes! That would be great.

lib/index.js Outdated Show resolved Hide resolved
@korelstar
Copy link
Contributor Author

  • Unit test required

I've added a test, now. The test uses a package ncu-test-peer (which has a peer dependency to ncu-test-return-version) for this purpose. We can transfer the ownership to you, if you want.

Yes! That would be great.

I've added you as maintainer. We can remove me now or after this PR is merged. Whatever you prefer.

@raineorshine raineorshine merged commit 791aa22 into raineorshine:main Apr 7, 2021
@raineorshine
Copy link
Owner

Published in v11.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants