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 #59

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Respect peer dependencies #59

wants to merge 1 commit into from

Conversation

korelstar
Copy link

This PR improves the upgrade of dependencies which are peer dependencies of other dependencies.

Scenario

Your app has a dependency to the package ncu-test-peer. ncu-test-peer has a peer dependency to the package ncu-test-return-version and requires version 1.x of this package. I.e., ncu-test-peer has the following package.json:

{
  "peerDependencies": {
    "ncu-test-return-version": "1.x"
  }
}

Hence, your app has the following package.json:

{
  "dependencies": {
    "ncu-test-peer": "1.0.0",
    "ncu-test-return-version": "1.0.0"
  }
}

Let us assume, that ncu-test-return-version has published versions 1.0.0, 1.1.0 and 2.0.0.

Current Situation (without this PR)

If you run the current release of npm-upgrade (3.0.0) for your app, you get an upgrade suggestion for ncu-test-return-version version 2.0.0. If you follow this suggestion, your app is broken, since ncu-test-peer's peer dependency requirement is not met (2.0.0 does not satisfy 1.x). If you deny this suggestion, you will stay at version 1.0.0, although there is a newer version 1.1.0 that satisfies the requirement 1.x.

Situation with this PR

This PR adds the new peer dependency check of npm-check-updates which was introduced in version 11.4.0 (see raineorshine/npm-check-updates#869). Now, npm-upgrade provides an upgrade suggestion for ncu-test-return-version version 1.1.0 -- which is the expected result for this scenario.

package-lock.json Show resolved Hide resolved
@@ -76,7 +76,8 @@ export const handler = catchAsyncError(async opts => {
.map(({ncuValue}) => ncuValue)
.join(',');
const currentVersions = ncu.getCurrentDependencies(packageJson, {dep: ncuDepGroups});
const latestVersions = await ncu.queryVersions(currentVersions, {versionTarget: 'latest'});
const peerDependencies = ncu.getPeerDependencies(currentVersions, {});
const latestVersions = await ncu.queryVersions(currentVersions, {versionTarget: 'latest', peerDependencies});
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a way to show some explanation to the user e.g. that latest released version of the dependency is v2 but the latest version that satisfies peerDependencies is v1.2?

Copy link
Author

Choose a reason for hiding this comment

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

Currently, with npm-check-updates, you can provide only one strategy at time. Hence, we would have to call ncu.queryVersions two times (one call with peerDependencies and one call without them). However, the package data will then be fetched two times from npm servers which doubles the process time. I don't think that this would be a good solution.

Copy link
Owner

Choose a reason for hiding this comment

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

which doubles the process time

We can fetch them in parallel. My concern here is it may introduce confusion when you're absolutely sure the latest version of the package is v2 but npm-upgrade tells you that the latest version is v1.2 without any explanation.

By the way, what does ncu.getPeerDependencies(currentVersions, {}) method actually do?

Copy link
Author

@korelstar korelstar Apr 13, 2021

Choose a reason for hiding this comment

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

We can fetch them in parallel. My concern here is it may introduce confusion when you're absolutely sure the latest version of the package is v2 but npm-upgrade tells you that the latest version is v1.2 without any explanation.

I understand that and I just had a deeper insight in the code. It looks like npm-check-updates caches the network responses, so a second call of ncu.queryVersions() doesn't trigger any new network requests and hence runs quite fast. Therefore, we can call it indeed twice, without any problems.

I just pushed a new commit which adds a hint, if the suggested version is not the latest version. It prints (ignored incompatible latest version X.Y.Z) in the output table.

For now, this is just a proof of concept. I think it would be better to show incompatible versions in a separate table just like the manually ignored versions. How about that?


By the way, what does ncu.getPeerDependencies(currentVersions, {}) method actually do?

For every package in currentVersions, it reads the local package.json (from the directory node_modules/package/) and extracts the section peerDependencies. All results are merged and sorted by package. See https://github.com/raineorshine/npm-check-updates/blob/v11.4.1/lib/index.js#L232

The result is added to the options, so that the filterPredicate can filter all versions that satisfy the peer dependencies.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I've changed the output for incompatible updates. Here are two examples:

First example

Checking for outdated dependencies for ".../package.json"...
[====================] 2/2 100%

New versions of active modules available:

  ncu-test-return-version   1.0.0   →   1.1.0 

Ignored incompatible updates (peer dependencies):

  ncu-test-return-version   1.0.0   →   2.0.0 

Second example

Checking for outdated dependencies for ".../package.json"...
[====================] 50/50 100%

New versions of active modules available:

  @nextcloud/dialogs                ^3.1.1   →     ^3.1.2 
  @nextcloud/router                 ^1.2.0   →     ^2.0.0 
  @nextcloud/vue                    ^3.8.0   →     ^3.9.0 
  @nextcloud/webpack-vue-config     ^4.0.1   →     ^4.0.2 
  @babel/core                     ^7.13.14   →   ^7.13.15 
  @babel/preset-env               ^7.13.12   →   ^7.13.15 
  core-js                          ^3.10.0   →    ^3.10.1 
  eslint                           ^7.23.0   →    ^7.24.0 
  eslint-plugin-vue                 ^7.8.0   →     ^7.9.0 
  webpack                          ^5.28.0   →    ^5.32.0 

Ignored incompatible updates (peer dependencies):

  css-loader               ^4.3.0   →    ^5.2.1 
  eslint-plugin-promise    ^4.3.1   →    ^5.1.0 
  sass-loader             ^10.1.1   →   ^11.0.1 

Ignored updates:

                  From          To   Ignored versions   Reason        
  vue-fragment   1.5.1   →   1.5.2        1.5.2         buggy version 

@korelstar korelstar requested a review from th0r April 18, 2021 12:07
@th0r
Copy link
Owner

th0r commented Apr 18, 2021

It's much better now, but it's still not clear what package is actually blocking other packages from updating. E.g. in my case I see the following:

Ignored incompatible updates (peer dependencies):

  @angular/animations         11.2.7   →   11.2.10
  @angular/common             11.2.7   →   11.2.10
  @angular/core               11.2.7   →   11.2.10
  @angular/platform-browser   11.2.7   →   11.2.10
  tslib                        2.1.0   →     2.2.0
  @angular/compiler           11.2.7   →   11.2.10
  typescript                   4.1.5   →     4.2.4
  webpack                     4.44.1   →    5.33.2

So I want to know what package prevents @angular packages from updating? Is it possible to get this information somehow?

@th0r
Copy link
Owner

th0r commented Apr 18, 2021

Another issue is I had to run npm-upgrade '@angular/*' 3 times in my project in order to make a minor Angular upgrade.
First time I was shown the following:

New versions of active modules available:

  @angular/cdk                        11.2.6   →    11.2.9
  @angular/forms                      11.2.7   →   11.2.10
  @angular/platform-browser-dynamic   11.2.7   →   11.2.10
  @angular/router                     11.2.7   →   11.2.10
  @angular/cli                        11.2.6   →    11.2.9
  @angular/compiler-cli               11.2.7   →   11.2.10

Ignored incompatible updates (peer dependencies):

  @angular/animations         11.2.7   →   11.2.10
  @angular/common             11.2.7   →   11.2.10
  @angular/core               11.2.7   →   11.2.10
  @angular/platform-browser   11.2.7   →   11.2.10
  @angular/compiler           11.2.7   →   11.2.10

After I have agreed with all updates and ran npm install and npm-upgrade again it showed the following:

New versions of active modules available:

  @angular/platform-browser   11.2.7   →   11.2.10
  @angular/compiler           11.2.7   →   11.2.10

Ignored incompatible updates (peer dependencies):

  @angular/animations   11.2.7   →   11.2.10
  @angular/common       11.2.7   →   11.2.10
  @angular/core         11.2.7   →   11.2.10

After doing same steps again:

New versions of active modules available:

  @angular/animations   11.2.7   →   11.2.10
  @angular/common       11.2.7   →   11.2.10

Ignored incompatible updates (peer dependencies):

  @angular/core   11.2.7   →   11.2.10

And only after 3rd time I've upgraded all the Angular packages.

It happens because some packages has other packages in their peer dependencies e.g. @angular/platform-browser@11.2.7 has @angular/animation@11.2.7 and @angular/common@11.2.7 in its peer dependencies list so these packages just can't be updated until platform-browser is updated. On the next step updated platform-browser@11.2.10 has animation@11.2.10 and common@11.2.10 in its peer deps so they can be updated now.

So the issue is current logic doesn't take into account that version of the peer dependency may change after the actual upgrade - it only takes current peer versions into account.

Another issue is only immediate peer deps are tracked which means an update to C@2 will be improperly suggested in the following situation:

<project>
 |-- C@^1 (update to v2 will be suggested but it's invalid as A -> B has C@^1 in its peer deps)
 |-- A
 |   |-- B
 |   |   |---C@^1 (peer)

@korelstar
Copy link
Author

You mentioned three aspects:

1. Show source of peer incompatibility

npm-check-updates doesn't provide this information, currently.

2. Multiple calls of npm-upgrade are necessary for complex dependencies

This is caused by your approach of splitting npm-upgrade and npm install. The only solution I can see is to combine both actions and run them repeatedly.

3. Consider deep peer dependencies

I'm not sure if this is required, because I think that A should have declared C as it's own peer dependency. In my point of view, A is broken if it does not do this. And as far as I understand the npm 7 approach correctly, the npm people have changed their mind and now have this view, too (see --strict-peer-deps flag). If you nevertheless want to consider deep peer dependencies, this should be fixed in npm-check-updates.

@th0r
Copy link
Owner

th0r commented Apr 19, 2021

This is caused by your approach of splitting npm-upgrade and npm install.

So what approach would work in your opinion?

@korelstar
Copy link
Author

korelstar commented Apr 23, 2021

An approach would be to automatically call npm install after updating package.json and then automatically check for updates again.

Another approach would be to execute npm view with the updated package version and get the new list of peer dependencies. Then, a check for updates can be done again without installing the packages. But there are two ways for user interaction:

Approach 1

The above actions are executed before the list of upgradable packages are shown to the user. This means, the user gets the following list in your example:

New versions of active modules available:

  @angular/cdk                        11.2.6   →    11.2.9
  @angular/forms                      11.2.7   →   11.2.10
  @angular/platform-browser-dynamic   11.2.7   →   11.2.10
  @angular/router                     11.2.7   →   11.2.10
  @angular/cli                        11.2.6   →    11.2.9
  @angular/compiler-cli               11.2.7   →   11.2.10
  @angular/platform-browser           11.2.7   →   11.2.10
  @angular/compiler                   11.2.7   →   11.2.10
  @angular/animations                 11.2.7   →   11.2.10
  @angular/common                     11.2.7   →   11.2.10

However, if the user says no to @angular/platform-browser but yes to @angular/animation, the resulting list of upgradable packages will be broken (i.e. not all peer dependencies are satisfied).

Approach 2

In this approach, we do this iteratively. When executing npm-upgrade, the following list is shown:

New versions of active modules available:

  @angular/cdk                        11.2.6   →    11.2.9
  @angular/forms                      11.2.7   →   11.2.10
  @angular/platform-browser-dynamic   11.2.7   →   11.2.10
  @angular/router                     11.2.7   →   11.2.10
  @angular/cli                        11.2.6   →    11.2.9
  @angular/compiler-cli               11.2.7   →   11.2.10

Ignored incompatible updates (peer dependencies):

  @angular/animations         11.2.7   →   11.2.10
  @angular/common             11.2.7   →   11.2.10
  @angular/core               11.2.7   →   11.2.10
  @angular/platform-browser   11.2.7   →   11.2.10
  @angular/compiler           11.2.7   →   11.2.10

But after the user has answered all questions, but before asking for updating package.json, npm view is executed for getting the new list of peer dependencies and the check for updates is done again with this list. This means, after answering the last update question, there will appear a status bar (every updated package has to be checked with npm view) and the user sees the next upgrade iteration and the next question appears. But the user just have to wait and does not need to call npm-upgrade multiple times.

@korelstar
Copy link
Author

korelstar commented Apr 25, 2021

I've made a PR for npm-check-updates in order to implement approach 1 directly over there: raineorshine/npm-check-updates#874

With that PR, it will be possible to add the reason to the list of incompatible updates. I will update this PR after the above PR has been merged and released.

Teaser 1:

New versions of active modules available:

  ncu-test-peer-update      1.0.0   →   1.1.0 
  ncu-test-return-version   1.0.0   →   1.1.0 

Ignored incompatible updates (peer dependencies):

  ncu-test-return-version   1.0.0   →   2.0.0   reason: ncu-test-peer-update requires 1.1.x 

? Update "ncu-test-peer-update" in package.json from 1.0.0 to 1.1.0? (Use arrow keys)
[...]

Teaser 2:

New versions of active modules available:

  @nextcloud/dialogs                ^3.1.1   →     ^3.1.2 
  @nextcloud/router                 ^1.2.0   →     ^2.0.0 
  @nextcloud/vue                    ^3.8.0   →     ^3.9.0 
  @nextcloud/webpack-vue-config     ^4.0.1   →     ^4.0.3 
  easymde                          ^2.14.0   →    ^2.15.0 
  markdown-it                      ^12.0.4   →    ^12.0.6 
  @babel/core                     ^7.13.14   →   ^7.13.16 
  @babel/preset-env               ^7.13.12   →   ^7.13.15 
  core-js                          ^3.10.0   →    ^3.11.0 
  eslint                           ^7.23.0   →    ^7.25.0 
  eslint-plugin-vue                 ^7.8.0   →     ^7.9.0 
  eslint-webpack-plugin             ^2.5.3   →     ^2.5.4 
  sass                             ^1.32.8   →   ^1.32.11 
  stylelint                       ^13.12.0   →   ^13.13.0 
  webpack                          ^5.28.0   →    ^5.35.1 

Ignored incompatible updates (peer dependencies):

  css-loader               ^4.3.0   →    ^5.2.4   reason: @nextcloud/webpack-vue-config requires ^4.3.0                                    
  eslint-plugin-promise    ^4.3.1   →    ^5.1.0   reason: @nextcloud/eslint-config requires ^4.2.1, eslint-config-standard requires ^4.2.1 
  sass-loader             ^10.1.1   →   ^11.0.1   reason: @nextcloud/webpack-vue-config requires ^10.0.5                                   

Ignored updates:

                  From          To   Ignored versions   Reason        
  vue-fragment   1.5.1   →   1.5.2        1.5.2         buggy version 

? Update "@nextcloud/dialogs" in package.json from ^3.1.1 to ^3.1.2? (Use arrow keys)
[...]

@korelstar
Copy link
Author

I'm moving some more code to npm-check-updates: raineorshine/npm-check-updates#876
After that was merged, we can call getIgnoredUpgrades from ncu and reduce the code in npm-upgrade.

@korelstar
Copy link
Author

Final update with much simplification was done. Ready for review and for merge 😁

@korelstar
Copy link
Author

@th0r Is there anything open that I should do?

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.

2 participants