fix: Do not upgrade if peerDependencies are not met #1421

rbnayax · 2024-06-02T10:32:47Z

fix: #1418

rbnayax · 2024-06-02T12:09:12Z

Hi @raineorshine

The CI here fails as well I will investigate soon.

Regarding the fix - this works according to my tests but it is very complicated. Would appreciate you're review.

overview::

Current process:

upgrade deps, while making sure current peer deps are kept
check if peer deps are updated (due to upgrades)
rereun the upgrade process, with updated pacakges and peers (step 1) as the upgraded peers might allow more upgrades

Suggested:

upgrade deps, while making sure current peer deps are kept
If updates causes the new packages list to be in peer violation due to peers update:
2.1. rereun the upgrade process with all the upgrades as it might sort itself and check if in violation again
2.2. rerun the upgrae process without upgrades that cause violation (only if peers udpated after removal)
2.3. return the results with the most new updates that is valid
else, check if peer deps are updated (due to upgrades)
3.1. rereun the upgrade process, with updated pacakges and peers (step 1) as the upgraded peers might allow more upgrades

rbnayax · 2024-06-02T17:19:40Z

Hi @raineorshine this is ready for initial review. It is not a straightforward fix and I would appreciate your input

raineorshine

Thanks for your contribution. I did my best to review, but it is admittedly complicated, as you pointed out. The unit test helps a lot though, so thanks for that. If there are any other test cases you want to add, that would help make this more robust.

In general, the code deserves some more thorough commenting. Otherwise it's going to be challenging to come back to this code in the future if any issues arise, or if the functionality needs to be extended.

raineorshine · 2024-06-02T20:59:11Z

src/lib/getIgnoredUpgrades.ts

+          .filter(
+            ([peer, peerSpec]) =>
+              upgradedPackagesWithPeerRestriction[peer] &&
+              !satisfies(upgradedPackagesWithPeerRestriction[peer], peerSpec),
+          )


Unless I am mistaken, you can filter by current[peer] && !intersects(current[peer], peerSpec), where intersects is from the semver package. This avoids the use of upgradedPackagesWithPeerRestriction to extract the lower bound of the version range.

Not sure if current is sufficient, or if you need to check { ...current, ...upgraded }.

(Note: Make sure to import the named import intersects rather than the default import in order to get the benefits of tree-shaking. You can see this done with satisfies.)

I'm using the same logic as in:

npm-check-updates/src/lib/runLocal.ts

Lines 183 to 192 in 0f17df2

if (options.peer) {

options.peerDependencies = await getPeerDependenciesFromRegistry(

Object.fromEntries(

Object.entries(current).map(([packageName, versionSpec]) => {

return [packageName, nodeSemver.minVersion(versionSpec)?.version ?? versionSpec]

}),

),

options,

)

}

I don't think intersect is the right way to go as it is not sufficient to make sure that peer dependency is met. For example, if package P1 spec is ^1.0.0 and P2 has P1 as peer with spec ^1.2.0, the 2 ranges will intersect but it will not mean that the peer dependency is met...

current is not sufficient, as we need to check against the versions that are currently upgraded.

Thoughts?

But if the P1 spec is ^1.0.0 (not 1.0.0) then npm will install 1.2.0 or later and the peer dependency will be met, no?

That will not be the case if your lcokfile was not updated and P1 was not upgraded due to a filter. But if you think that will be better I will make the change, looking forward to your opinion

npm-check-updates is generally just concerned with package.json versions, and assumes that the user will run npm install and manage the lockfile through npm. The lockfile is basically always out of date after running ncu -u, so I don't see that as a problem. It would be a problem if npm-check-updates upgraded a version range to something that was incompatible though. If you think that's the case, please let me know. I didn't notice that the existing code uses the same minVersion trick to get the bottom of the range.

I'm having a hard time picturing the situation you described with a filter on P1. If you think that's a problem, we should add a unit test that covers that. It is admittedly difficult to reason about this abstractly, at least for me.

Im not sure I can write a test for it. I changed to intresect in both places where I used minVersion

src/lib/upgradePackageDefinitions.ts

rbnayax · 2024-06-08T10:43:02Z

@raineorshine I pushed all the fixes, looking forward to your next review

raineorshine

Getting closer. Thanks for your effort!

raineorshine · 2024-06-12T18:48:29Z

src/lib/upgradePackageDefinitions.ts

+      return [packageName, minVersion(versionSpec)?.version ?? versionSpec]
+    }),
+  )
+  const filteredUpgradedPeerDependencies = { ...upgradedPeerDependencies }


There's a little unnecessary complexity in checkIfInPeerViolation due to filteredUpgradedPeerDependencies. Let me try to explain.

The pickBy below is used to filter down filteredUpgradedDependencies to only those that satisfy the peerDependencies. At the same time, it needs to filter down upgradedPeerDependencies by exactly the same dependencies. To do this, the current implementation introduces a mutable variable filteredUpgradedPeerDependencies which it modifies to keep in alignment with the pickBy accumulator, i.e. filteredUpgradedPeerDependencies. This requires the developer to observe the delete operation and then reason about the pickBy behavior to understand the relationship between filteredUpgradedPeerDependencies and filteredUpgradedDependenciesAfterPeers.

Instead, it would be cleaner to change the pickBy to a reduce and filter down filteredUpgradedDependencies and upgradedPeerDependencies at the same time. This avoids the mutable variable and shows both collections being filtered down at the same time through the accumulator.

Let me know if this makes sense :).

Honestly, I can't see how you can filter down both at the same time. Their keys are overlapping and not contained in one another and as such there is no single set of keys I can reduce on. If you can post a snippet showing how, I would appreciate it.

That being said, I did change it a bit to be cleaner without deleting keys, let me know what you think

src/lib/upgradePackageDefinitions.ts

rbnayax · 2024-06-15T17:15:59Z

@raineorshine I pushed all the fixes, looking forward to your next review

raineorshine · 2024-06-20T16:47:34Z

Thanks! I have some work responsibilities right now, but I'll review as soon as I can.

rbnayax · 2024-06-23T07:55:21Z

Thanks! I have some work responsibilities right now, but I'll review as soon as I can.

Thanks I appreciate it

rbnayax · 2024-06-30T07:02:00Z

Thanks! I have some work responsibilities right now, but I'll review as soon as I can.

Hi @raineorshine just a ping to remind you in case you forgot :-)

raineorshine · 2024-07-04T20:48:15Z

src/lib/upgradePackageDefinitions.ts

+  const violated = violatedDependencies.size > 0
+  let filteredUpgradedPeerDependencies = upgradedPeerDependencies
+  if (violated) {
+    filteredUpgradedPeerDependencies = pickBy(upgradedPeerDependencies, (spec, dep) => !violatedDependencies.has(dep))


Is !violatedDependencies.has(dep) the same as !filteredUpgradedDependencies[dep] || !filteredUpgradedDependenciesAfterPeers[dep]?

If so, we should be able to get rid of violatedDependencies. If you don't see it, let me know and I'll take a stab at it.

not quite but I changed it

raineorshine · 2024-07-04T21:01:02Z

src/lib/upgradePackageDefinitions.ts

    }
+    let rerunResult: UpgradePackageDefinitionsResult
+    let runIndex = 0
+    do {


I'm struggling to understand this do-while loop. Something about base case being both when runIndex is 0 and 1, and the continual reassignment of rerunResult makes it challenging to read.

Any ideas how to make this logic more obvious?

I gave it another shot

raineorshine · 2024-07-07T17:19:22Z

Thanks, I'll take a look soon.

Would you mind rebasing on main? I just merged your other PR and there are some merge conflicts. Thank you.

rbnayax · 2024-07-07T17:21:26Z

Thanks, I'll take a look soon.

Would you mind rebasing on main? I just merged your other PR and there are some merge conflicts. Thank you.

of course already on it

rbnayax · 2024-07-07T18:02:31Z

Thanks, I'll take a look soon.

Would you mind rebasing on main? I just merged your other PR and there are some merge conflicts. Thank you.

Done

raineorshine · 2024-08-07T12:14:52Z

Sorry about the delay! I've had work responsibilities and have not found time to review this yet.

rbnayax · 2024-08-18T06:56:55Z

@raineorshine how are you? Can we expect this to be reviewed soon? It will help my team a lot to have that feature in production

raineorshine · 2024-08-18T07:04:52Z

Yes. I've been at a conference this week, but I will try to get to it soon.

… are not currently met raineorshine#1418

raineorshine

Looks good! Thanks for your patience.

Will release tonight or tomorrow.

rbnayax marked this pull request as draft June 2, 2024 13:27

rbnayax marked this pull request as ready for review June 2, 2024 17:18

raineorshine requested changes Jun 2, 2024

View reviewed changes

raineorshine changed the title ~~fix: Is there a way to not upgrade a package if it's peer depndencies are not currently met #1418~~ fix: Do not upgrade if peerDependencies are not met Jun 2, 2024

raineorshine requested changes Jun 12, 2024

View reviewed changes

raineorshine requested changes Jul 4, 2024

View reviewed changes

rbnayax force-pushed the 1418 branch from 4ca6b54 to a1d00fe Compare July 7, 2024 17:55

rbnayax requested a review from raineorshine July 15, 2024 07:57

rbnayax force-pushed the 1418 branch from a1d00fe to 4153fee Compare August 7, 2024 09:51

rbnayax force-pushed the 1418 branch 2 times, most recently from 6d7d9f6 to 9e813c0 Compare August 11, 2024 15:14

fix: Is there a way to not upgrade a package if it's peer depndencies…

e432959

… are not currently met raineorshine#1418

raineorshine force-pushed the 1418 branch from 9e813c0 to e432959 Compare August 30, 2024 20:43

raineorshine approved these changes Aug 30, 2024

View reviewed changes

raineorshine merged commit 6fb4285 into raineorshine:main Sep 1, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Do not upgrade if peerDependencies are not met #1421

fix: Do not upgrade if peerDependencies are not met #1421

rbnayax commented Jun 2, 2024

rbnayax commented Jun 2, 2024

rbnayax commented Jun 2, 2024

raineorshine left a comment

raineorshine Jun 2, 2024

rbnayax Jun 8, 2024

raineorshine Jun 12, 2024

rbnayax Jun 12, 2024

raineorshine Jun 12, 2024

rbnayax Jun 15, 2024

rbnayax commented Jun 8, 2024

raineorshine left a comment

raineorshine Jun 12, 2024

rbnayax Jun 15, 2024

rbnayax commented Jun 15, 2024

raineorshine commented Jun 20, 2024

rbnayax commented Jun 23, 2024

rbnayax commented Jun 30, 2024

raineorshine Jul 4, 2024

rbnayax Jul 7, 2024 •

edited

Loading

raineorshine Jul 4, 2024 •

edited

Loading

rbnayax Jul 7, 2024

raineorshine commented Jul 7, 2024

rbnayax commented Jul 7, 2024

rbnayax commented Jul 7, 2024

raineorshine commented Aug 7, 2024

rbnayax commented Aug 18, 2024

raineorshine commented Aug 18, 2024

raineorshine left a comment

	if (options.peer) {
	options.peerDependencies = await getPeerDependenciesFromRegistry(
	Object.fromEntries(
	Object.entries(current).map(([packageName, versionSpec]) => {
	return [packageName, nodeSemver.minVersion(versionSpec)?.version ?? versionSpec]
	}),
	),
	options,
	)
	}

fix: Do not upgrade if peerDependencies are not met #1421

fix: Do not upgrade if peerDependencies are not met #1421

Conversation

rbnayax commented Jun 2, 2024

rbnayax commented Jun 2, 2024

rbnayax commented Jun 2, 2024

raineorshine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbnayax commented Jun 8, 2024

raineorshine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbnayax commented Jun 15, 2024

raineorshine commented Jun 20, 2024

rbnayax commented Jun 23, 2024

rbnayax commented Jun 30, 2024

Choose a reason for hiding this comment

rbnayax Jul 7, 2024 • edited Loading

Choose a reason for hiding this comment

raineorshine Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raineorshine commented Jul 7, 2024

rbnayax commented Jul 7, 2024

rbnayax commented Jul 7, 2024

raineorshine commented Aug 7, 2024

rbnayax commented Aug 18, 2024

raineorshine commented Aug 18, 2024

raineorshine left a comment

Choose a reason for hiding this comment

rbnayax Jul 7, 2024 •

edited

Loading

raineorshine Jul 4, 2024 •

edited

Loading