JamieMason / syncpack

Consistent dependency versions in large JavaScript Monorepos.
https://jamiemason.github.io/syncpack/
MIT License
1.46k stars 51 forks source link

Unexpected Behavior of `lint-semver-ranges` and `list-mismatches` Commands with `versionGroup` and `semverGroup` Policies in Config #221

Open ardelato opened 6 months ago

ardelato commented 6 months ago

Description

[!Note] I created a reproducible repo that includes more details -- https://github.com/ardelato/syncpack-config-ooo-repo/tree/main.


I've encountered unexpected behaviors with lint-semver-ranges and list-mismatches commands when using a configuration file with both versionGroup and semverGroup policies.

There seems to be an underlying behavior that is not communicated properly or the behavior itself is a bug.

Issues 1. **`lint-semver-ranges` Command:** - **Expected:** The command should only consider `semverGroup` policies and flag any `peerDependencies` not using the `^` range as errors. ![image](https://github.com/JamieMason/syncpack/assets/22064420/dad8de8d-4d1e-4c9e-934f-ef9ff3e930e5) - **Actual:** Despite only `semverGroup` policies being logged, a `PinnedMismatch` type validation is taking into account and marks one of the prior `react` `peerDependencies` as valid. ![image](https://github.com/JamieMason/syncpack/assets/22064420/4130be4e-2c71-4100-9c94-ba32314354f1) 2. **`list-mismatches` Command:** - **Expected:** The command should solely consider `versionGroup` policies for identifying mismatches. For instance, mismatches like the `lodash` `peerDependency` in `packageB` should not be flagged if they do not fall under any `versionGroup` range definition. ![image](https://github.com/JamieMason/syncpack/assets/22064420/2001d25e-dee1-4da6-8bb6-c1074017a112) - **Actual:** The `lodash` `peerDependency` in `packageB` was flagged as an error when using `withVersionGroup.mjs`, indicating a possible unintended consideration of the `semverGroup` policies. ![image](https://github.com/JamieMason/syncpack/assets/22064420/88a02e6e-f46e-4c02-9ec0-e9fbf0b839d3)

I tried looking at the syncpack docs to see if there was any mention that could explain this behavior and only found this - https://jamiemason.github.io/syncpack/guide/getting-started/

image

From this callout, I would still have expected the relevant group's policies to take precedence given the output of the commands.



Overall, Syncpack is a very handy tool so if you need further information or help, please let me know!

JamieMason commented 5 months ago

First of all Angel, I think this might be the best bug report I've ever received 😆 Thank you for all this detail.

The timing of this is great actually as I've been struggling a little with what to do about these kinds of things for a long while and it needs nailing down. Questions like what should happen when a semver group is defined but that semver group causes what would otherwise be identical versions to subtly differ – should the version or semver group win out over the other or can they both work together – some scenarios might want the versions to be aligned but then the semver rules to be applied afterwards, others might want the versions to overrule the semver rules, others others might want the semver rules to never be broken.

It'll take some time to digest all of this info. I want to incorporate your cases into the test suite, and I think possibly changes to syncpack might be needed to handle different scenarios around whether version or semver groups "win" in different cases, and also some edge cases about what should happen when you run lint vs fix – I won't muddy this issue by going into that though.

One thing I can answer right now for one part of it, there are tests that show that pinned versions take precedence.

A lot of your questions are still unanswered but I'll get back to you when I can.

ardelato commented 5 months ago

First of all Angel, I think this might be the best bug report I've ever received 😆 Thank you for all this detail.

Haha thank you mate. I was previously a Software QA so I've learned how to write good bug reports and now know the value of receiving a good bug report as a Developer 😅. I'm glad you found the amount of detail valuable.


some scenarios might want the versions to be aligned but then the semver rules to be applied afterwards, others might want the versions to overrule the semver rules, others others might want the semver rules to never be broken.

I would imagine trying to please everyone's specific use case would be a hassle. On that note, could things not be simplified to help improve the flexibility of the tool?

For instance, could the SemverGroup rules not be pulled into VersionGroup, thus having a single group? From the documentation, SemverGroup really only has a range rule since both groups have an ignore rule. That said SemverGroup's range rule seems similar to VersionGroup's sameRange rule. If VersionGroup were to have a rule like the SemverGroup range rule then the SemverGroup could be dropped. Thus only one array of rules needs to be take precedence.

In addition, if the first matching instance logic were to be dropped, then the policies could be more flexible and reapplied if need be. The order in which rules are applied would now only be left to the expectation of FIFO. Thus things like pinning a version and ensuring peerDependencies follow a semver range could all be done in a single run.

All that said, I assume there are reasons for the two rule groups and the first matching instance logic, but I have not taken a look at the source code or the commits so take my thoughts with a grain of salt.


One thing I can answer right now for one part of it, there are tests that show that pinned versions take precedence.

Ahh dope! Thank you mate!


A lot of your questions are still unanswered but I'll get back to you when I can.

No worries at all mate. I understand its just you maintaining this tool and you probably have your hands full porting it over to Rust -- https://github.com/JamieMason/syncpack/issues/222.