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(core): ignore missing snapTo dependencies #173

Open
JamieMason opened this issue Nov 24, 2023 · 7 comments
Open

fix(core): ignore missing snapTo dependencies #173

JamieMason opened this issue Nov 24, 2023 · 7 comments

Comments

@JamieMason
Copy link
Owner

Originally posted by @silvaj8 in #87 (comment):

@JamieMason I'm testing the newest version 12.0.0-alpha.0 and I'm still facing the issue @pastelsky referred above (if I understood it correctly). If you prefer I can create a separate issue for this, but since this is related to this thread of comments and related to the testing of an alpha version, I thought that discussing it here would suffice.

Description

I need all local packages to use whatever version some specific local packages (a.k.a. "apps") are using of the local packages. And for the other local packages (that are not used by any "app"), I need them to use workspace protocol (workspace:*). So, I approached it like this:

{
  label: "Ensure all packages use whatever version the apps are using",
  packages: ["**"],
  dependencies: ["$LOCAL"],
  dependencyTypes: ["**"],
  snapTo: ["appA", "appB", "appC"],
},
{
  label: "Ensure all packages use the workspace protocol for the local packages",
  packages: ["**"],
  dependencies: ["$LOCAL"],
  dependencyTypes: ["!local"],
  pinVersion: "workspace:*",
},

Output

But this causes an error when a local package is not used by an app, something like this:

! failed to get snapped to version for packageX using ["appA","appB","appC"]
✘ packageX 0.7.1 → ??? packages/packageX/package.json > version [missing snapTo version]
  no package in this groups .snapTo array depend on packageX
✘ packageX workspace:* → ??? packages/packageY/package.json > devDependencies [missing snapTo version]
  no package in this groups .snapTo array depend on packageX
✘ packageX workspace:* → ??? packages/packageZ/package.json > devDependencies [missing snapTo version]
  no package in this groups .snapTo array depend on packageX

Expected output

I was expecting for snapTo to gracefully skip the local packages that are not used by an app, so I don't need to enumerate each local package used inside the apps on the dependencies array, which is my current workaround.

Context

I can't share any repo, but I will try to describe my context in the best and simplest way possible to check if I'm approaching my necessity incorrectly. In my shared monorepo I have almost 30 packages and 3 "apps", and I use the workspace protocol for every package, with some exceptions in the apps. These apps have some particularities:

  • They are not like packages, as they are not to be used by other packages;
  • They can use any of the local packages, but they cannot use the workspace protocol, only semVer (for some irrelevant reason that I can't control related to NPM vs Yarn). And the same applies for the packages that the apps use, it's recursive. Example: appA uses packageA, and packageA uses packageB, so in the package.json of appA, packageA needs to use semVer, and in the package.json of packageA, packageB also needs to use semVer. In summary, we can't use the workspace protocol in the package.json of the apps and in its dependencies' package.json;
  • Since we want to use the same version for the same local package across the monorepo, these packages will have to use semVer even in packages that are not used by an app.
@JamieMason
Copy link
Owner Author

Thanks @silvaj8, there'll be a bit of back and forth to figure that out together so I've created this issue. I'm at my day job but will get back to you when I can.

It won't solve all of your issue as part of it may need changes to syncpack, but one thing I see is that your 2nd version group there won't match anything because the one before it is greedier, see the section "Order groups by most to least specific" at https://jamiemason.github.io/syncpack/guide/getting-started/.

We can discuss the rest properly soon.

@silvaj8
Copy link

silvaj8 commented Nov 24, 2023

It won't solve all of your issue as part of it may need changes to syncpack

I understand that the recursive part isn't something that syncpack can perform, and I'm ok with that. But just by gracefully skipping packages that are not found in any of the packages provided in the snapTo array, it would help me a lot.

one thing I see is that your 2nd version group there won't match anything because the one before it is greedier

Yeah, it is greedier, but the way I see it, I want it that way. For example, in the apps, from the 30 packages I have in the monorepo, only around 10 are used in the apps right now. So I want those 10 to be used across the monorepo as they are used in the apps (with semVer), and the remaining 20 with the workspace protocol, and that's where the 2nd version group comes into effect.

@JamieMason
Copy link
Owner Author

JamieMason commented Nov 24, 2023

But just by gracefully skipping packages that are not found in any of the packages provided in the snapTo array, it would help me a lot.

Something can be done for sure, what I'm concerned about though initially is that if I change it to not error, genuine issues could be missed in other projects and no one would know.

A bigger job but one I think could work is to expose config for each status code to decide whether to error, warn, or stay silent.

Yeah, it is greedier, but the way I see it, I want it that way.

What I mean by this is that the 2nd version group will never match anything, you could delete it and it would make no difference.

  • The only bits which are different are dependencyTypes: ["**"] and dependencyTypes: ["!local"].
  • The first group matches every dependency type, so every instance of a local package (dependencies: ["$LOCAL"]) in every package (packages: ["**"]) will be assigned to the first group.
  • After that there will be zero instances left to go into the second group, they're all already in the first one.

@silvaj8
Copy link

silvaj8 commented Nov 24, 2023

what I'm concerned about though initially is that if I change it to not error, genuine issues could be missed in other projects and no one would know

I understand. If that's a real concern, we need to keep the current configuration with the same output, and only those who want this feature I'm suggesting, would need to change their configuration.

A bigger job but one I think could work is to expose config for each status code to decide whether to error, warn, or stay silent.

That's a possible approach and flexible. I was thinking of a more simple approach and specific to the snapTo feature, but maybe I'm not seeing the bigger picture. What do you think of adding a property in the versionGroups object called isRelaxed or something like that, being the default value false to address your concern? For example, my configuration would become:

{
  label: "Ensure all packages use whatever version the apps are using",
  packages: ["**"],
  dependencies: ["$LOCAL"],
  dependencyTypes: ["**"],
  snapTo: ["appA", "appB", "appC"],
  isRelaxed: true,
},

The first group matches every dependency type, so every instance of a local package (dependencies: ["$LOCAL"]) in every package (packages: ["**"]) will be assigned to the first group.

I think that here is where we're thinking differently. With the "relaxed" snapTo feature, I would expect for the first group to match only the local packages that are used by the packages provided in the snapTo array. And then, the remaining local packages would be matched in the second version group.

@JamieMason
Copy link
Owner Author

I'm with you, some good options to consider here thanks. I have a few things to do this weekend and coming week but let me have a think and I'll pick up with you again when I can.

Thanks a lot for your input.

@JamieMason JamieMason changed the title Question on configuring snapTo groups feat(core): add overridable severity levels Dec 30, 2023
@silvaj8
Copy link

silvaj8 commented Dec 30, 2023

Hi @JamieMason, I've been thinking about this and I noticed now that you've changed the title of the issue/question. I don't think that adding an option to override severity levels will solve this issue, only mask it. I believe the feature you identified could be really useful in some cases, so don't disregard it, but not for my use case (and I believe others as well).

I think the "problematic" we're aiming to solve was introduced when the configuration dependencies: ["$LOCAL"] was added. In my opinion, this configuration together with the snapTo feature, should be always interpreted as "the local dependencies that are found in the dependencies given by snapTo".

what I'm concerned about though initially is that if I change it to not error, genuine issues could be missed in other projects and no one would know
#173 (comment)

I understand your concern, but I don't really see the usefulness of outputting an error when a dependency is not found in a "snapped to" dependency. The snapTo feature aims to pin the version of given dependencies to the version used by other dependencies, so it should output an error only when the version is not the same, and maybe a warning if a dependency is not found, although I don't see the use of it. Do you have an example where it could be important to output an error/warning when a package is not found?

When the dependencies are specified, then I guess it could make sense to output something if a dependency is not found, since it is due to user input (the user has an extra dependency or forgot to add one to the snapTo array). But when "$LOCAL" is given, which could correspond to a lot of dependencies (30 in my case), then I think it should be relaxed when a dependency is not found.

I understand that this would mean we would have two different default behaviours and that's probably not cool, so maybe we could have a configuration to specify we wanted the relaxed behaviour instead of the default one or change the default one, which I understand you could be against.

What do you think?

@JamieMason
Copy link
Owner Author

JamieMason commented Dec 30, 2023

Thanks @silvaj8, I think you're right and it would be better to just not throw for this scenario, for the reasons you gave. I'll think over severity levels separately, whether they're really needed at all.

@JamieMason JamieMason changed the title feat(core): add overridable severity levels fix(core): ignore missing snapTo dependencies Dec 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants