-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Comments
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. |
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
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. |
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.
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.
|
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.
That's a possible approach and flexible. I was thinking of a more simple approach and specific to the {
label: "Ensure all packages use whatever version the apps are using",
packages: ["**"],
dependencies: ["$LOCAL"],
dependencyTypes: ["**"],
snapTo: ["appA", "appB", "appC"],
isRelaxed: true,
},
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 |
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. |
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
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 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 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? |
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. |
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:Output
But this causes an error when a local package is not used by an app, something like this:
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 thedependencies
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:
appA
usespackageA
, andpackageA
usespackageB
, so in thepackage.json
ofappA
,packageA
needs to use semVer, and in thepackage.json
ofpackageA
,packageB
also needs to use semVer. In summary, we can't use the workspace protocol in thepackage.json
of the apps and in its dependencies'package.json
;The text was updated successfully, but these errors were encountered: