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: Do not upgrade if peerDependencies are not met #1421

Merged
merged 1 commit into from
Sep 1, 2024

Conversation

rbnayax
Copy link
Contributor

@rbnayax rbnayax commented Jun 2, 2024

fix: #1418

@rbnayax
Copy link
Contributor Author

rbnayax commented Jun 2, 2024

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:

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

Suggested:

  1. upgrade deps, while making sure current peer deps are kept
  2. 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
  3. 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 rbnayax marked this pull request as draft June 2, 2024 13:27
@rbnayax rbnayax marked this pull request as ready for review June 2, 2024 17:18
@rbnayax
Copy link
Contributor Author

rbnayax commented Jun 2, 2024

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

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.

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.

Comment on lines 46 to 50
.filter(
([peer, peerSpec]) =>
upgradedPackagesWithPeerRestriction[peer] &&
!satisfies(upgradedPackagesWithPeerRestriction[peer], peerSpec),
)
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using the same logic as in:

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
src/lib/upgradePackageDefinitions.ts Outdated Show resolved Hide resolved
src/lib/upgradePackageDefinitions.ts Outdated Show resolved Hide resolved
src/lib/upgradePackageDefinitions.ts Outdated Show resolved Hide resolved
src/lib/upgradePackageDefinitions.ts Outdated Show resolved Hide resolved
src/lib/upgradePackageDefinitions.ts Outdated Show resolved Hide resolved
src/lib/upgradePackageDefinitions.ts Outdated Show resolved Hide resolved
src/lib/upgradePackageDefinitions.ts Outdated Show resolved Hide resolved
@raineorshine 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
@rbnayax
Copy link
Contributor Author

rbnayax commented Jun 8, 2024

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

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.

Getting closer. Thanks for your effort!

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
src/lib/upgradePackageDefinitions.ts Outdated Show resolved Hide resolved
src/lib/upgradePackageDefinitions.ts Outdated Show resolved Hide resolved
src/lib/upgradePackageDefinitions.ts Outdated Show resolved Hide resolved
@rbnayax
Copy link
Contributor Author

rbnayax commented Jun 15, 2024

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

@raineorshine
Copy link
Owner

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

@rbnayax
Copy link
Contributor Author

rbnayax commented Jun 23, 2024

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

Thanks I appreciate it

@rbnayax
Copy link
Contributor Author

rbnayax commented Jun 30, 2024

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@rbnayax rbnayax Jul 7, 2024

Choose a reason for hiding this comment

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

not quite but I changed it

}
let rerunResult: UpgradePackageDefinitionsResult
let runIndex = 0
do {
Copy link
Owner

@raineorshine raineorshine Jul 4, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave it another shot

@raineorshine
Copy link
Owner

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
Copy link
Contributor Author

rbnayax commented Jul 7, 2024

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
Copy link
Contributor Author

rbnayax commented Jul 7, 2024

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
Copy link
Owner

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

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

rbnayax commented Aug 18, 2024

@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
Copy link
Owner

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

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.

Looks good! Thanks for your patience.

Will release tonight or tomorrow.

@raineorshine raineorshine merged commit 6fb4285 into raineorshine:main Sep 1, 2024
7 checks passed
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.

Do not upgrade a package if its peer dependencies are not currently met
2 participants