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

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

Open
ardelato opened this issue May 30, 2024 · 2 comments

Comments

@ardelato
Copy link
Contributor

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

    • 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

  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

    • 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

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

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

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 -- #222.

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

No branches or pull requests

2 participants