Constraints don't take devDependencies into account

[arcanis]

Describe the bug

The constraint feature implemented by the plugin-constraints package doesn't insert devDependencies into the fact database, so they can't be matched by the rules.

To Reproduce

package.json:

{
  "devDependencies": {
    "lodash": "*"
  }
}

constraints.pro:

gen_invalid_dependency(WorkspaceCwd, DependencyName, _) :-
  workspace_has_dependency(WorkspaceCwd, DependencyName).

Running yarn constraints check should report an error (since the constraints don't allow any dependency to pass), but since lodash is declared as a devDependency nothing is reported.

Additional context

The fix would be to change the signature of workspace_has_dependency in order to add the dependency type. This would also be useful for peer dependencies as well (and would allow to build rules such as "reject devDependency if the package also list a dependency of the same name").

[bgotink]

I'd be willing to look into this!

How far should this support for dev dependencies go? In my opinion only exposing them via workspace_has_dependency isn't enough.
Here are two examples of rules that I believe we should support:

• if a workspace has a dependency on package Y (any version), it shouldn't be allowed to have a devDependency on that same package Y
• if a workspace has a peerDependency on package Y version Z, it should also have a devDependency on package Y version Z

We could support all of these by adding a new parameter to the workspace_has_dependency, gen_invalid_dependency and gen_enforced_dependency_range predicates:

workspace_has_dependency(WorkspaceCwd, DependencyIdent, DependencyRange, DependencyType).
gen_enforced_dependency_range(WorkspaceCwd, DependencyIdent, DependencyRange, DependencyType).
gen_invalid_dependency(WorkspaceCwd, DependencyIdent, DependencyType, Reason).

The example rules above would then be

% Dissallow packages in devDependencies if they're already in dependencies
gen_enforced_dependency_range(WorkspaceCwd, DependencyIdent, null, devDependencies):-
  workspace_has_dependency(WorkspaceCwd, DependencyIdent, _, dependencies).

% Enforce packages to be listed in devDependencies if listed in peerDependencies
gen_enforced_dependency_range(WorkspaceCwd, DependencyIdent, DependencyRange, devDependencies):-
  workspace_has_dependency(WorkspaceCwd, DependencyIdent, DependencyRange, peerDependencies).

[bgotink]

I actually started looking into this over the weekend, and let's just say I went all the way down the rabbit-hole. I'm not sure where I should write this down so I'm posting it here, for now…

An overview of why I'm interested: I've got a couple of repositories at work, all using yarn workspaces. Dependency management is becoming a mess: wrong versions are being added quite frequently, mostly because multiple PRs are open at the same time. If one of those PRs updates all dependencies on a certain package but another PR adds a new dependency to that same package, things go sideways.
I've tried linters for package.json files, but these never quite worked out. For starters, there's no extensible package linter available. A bigger issue than that is that this isn't a codestyle issue, violations are bugs that should be fixed instantly.
Once I saw the plugin-constraints package, I was sold. It's very open to custom rules without requiring the boilerplate of writing rule classes and configuration. One downside though: it's berry.

So, long story short, I have created package-constraints. It is heavily based on the plugin-constraints package in this repo, with the following changes:

• It supports yarn workspaces (yarn v1) as well as regular packages
• it implements this issue, [Feature] Expose extra information in plugin-constraints #31, and the suboptimal feature proposed in [Feature] debuggable constraints #32
• it supports outputting rule violations in the TAP format, allowing CI tools to pick it up
• Predicate names are changed from workspace_* to package_*, because in non-yarn workspaces packages the name package makes more sense
• Package names are used as identifiers for the package rather than the cwd
• There's an extra dependency_type predicate that matches three values: dependencies, peerDependencies and devDependencies

I've succeeded in implementing constraints in two of our repositories at work, catching about twenty violations in the process. Most were missing devDependencies but a couple of violations were actual bugs.

For reference, this is the constraints file I've been using so far:

% Data data data

pin('@angular/cdk', Version, '^'):- root_dependency_version('@angular/cdk', Version).
pin('@angular/cli', Version, '^'):- root_dependency_version('@angular/cli', Version).
pin('@angular-devkit/schematics-cli', null, '~').
pin('@angular-devkit/architect', Version, '~'):- root_dependency_version('@angular-devkit/architect', Version).
pin('@angular-devkit/build-', Version, '~'):- root_dependency_version('@angular-devkit/architect', Version).
pin('@angular-devkit/', Version, '^'):- root_dependency_version('@angular/cli', Version).
pin('@angular', Version, '^'):- root_dependency_version('@angular/core', Version).

pin('rxjs', Version, '^'):- root_dependency_version('rxjs', Version).
pin('tslib', '1.9.3', '^').

require_dependency_type('tslib', dependencies).

% Helpers

root_dependency_version(DependencyName, DependencyVersion):-
  root_package(PackageName),
  package_has_dependency(PackageName, DependencyName, DependencyVersion, devDependencies), !.

version_matches(Range, Version):-
  atom_concat('^', Version, Range), !.
version_matches(Range, Version):-
  atom_concat('~', Version, Range), !.
version_matches(Range, Version):-
  Range = Version.

find_pinned_version(PackageName, null, DependencyType):-
  dependency_type(DependencyType),
  pin(PinnedPackage, null, _),
  sub_atom(PackageName, 0, _, _, PinnedPackage).
find_pinned_version(PackageName, Version, devDependencies):-
  pin(PinnedPackage, PinnedVersion, _),
  sub_atom(PackageName, 0, _, _, PinnedPackage),
  Version = PinnedVersion.
find_pinned_version(PackageName, Version, DependencyType):-
  dependency_type(DependencyType),
  DependencyType \= devDependencies,
  pin(PinnedPackage, PinnedVersion, PinnedPrefix),
  sub_atom(PackageName, 0, _, _, PinnedPackage),
  atom_concat(PinnedPrefix, PinnedVersion, Version).

% Our actual rules

% This rule prevents packages from depending on non-workspace versions of workspace packages
gen_enforced_dependency_range(PackageName, DependencyName, DependencyVersion, DependencyType) :-
  package_has_dependency(PackageName, DependencyName, _, DependencyType),
  package_version(DependencyName, DependencyVersion).

% This rule forces usage of the correct dependency type
gen_enforced_dependency_range(PackageName, DependencyName, Version, DependencyType) :-
  require_dependency_type(DependencyName, DependencyType),
  package_has_dependency(PackageName, DependencyName, Version, _).

% This rule requires peerDependencies to be listed as devDependency
gen_enforced_dependency_range(PackageName, DependencyName, DependencyRange, devDependencies) :-
  package_has_dependency(PackageName, DependencyName, PeerDependencyRange, peerDependencies),
  version_matches(PeerDependencyRange, DependencyRange).

% This rule requires use of correct versions
% it potentially requires ^ or ~ for runtime dependencies except the demo which must pin all versions
gen_enforced_dependency_range(PackageName, DependencyName, DependencyVersion, DependencyType) :-
  \+(private_package(PackageName)),
  package_has_dependency(PackageName, DependencyName, _, DependencyType),
  once(find_pinned_version(DependencyName, PinnedVersion, DependencyType)),
  PinnedVersion \== null,
  DependencyVersion = PinnedVersion.
gen_enforced_dependency_range(PackageName, DependencyName, DependencyVersion, DependencyType) :-
  private_package(PackageName),
  package_has_dependency(PackageName, DependencyName, _, DependencyType),
  once(find_pinned_version(DependencyName, PinnedVersion, devDependencies)),
  PinnedVersion \== null,
  DependencyVersion = PinnedVersion.

[arcanis]

I'm happy to hear that constraints solved actual problems in your repo. I'm pretty sure that after some refinement they can become one of the most powerful tools in the Yarn toolset 😃

Once I saw the plugin-constraints package, I was sold. It's very open to custom rules without requiring the boilerplate of writing rule classes and configuration. One downside though: it's berry.

I won't focus on this too much - I find it awesome that you went ahead and improved the process. That said, note that Berry is meant to be mostly backward compatible with Yarn (it uses the same workspace definition, for example, with less bugs), so I'd recommend giving it a try when you get the chance 🙂

We could support all of these by adding a new parameter to the workspace_has_dependency, gen_invalid_dependency and gen_enforced_dependency_range predicates:

Yep, that's how I'd do it as well. I agree that adding a predicate like workspace_dependency_type would make us lose information if the dependency is listed multiple times. The main worry I'd have would be to complexify the two gen_ rules - hopefully DependencyType is the last parameter we'd need to add.

Package names are used as identifiers for the package rather than the cwd

I'd advise against it - workspaces aren't required to have names. Similarly, two workspaces may have the same name but different versions (I admit it's a bit of a fringe case, but it may happen). The cwd is the only identifier that will be guaranteed to be unique across a project.

[bgotink]

note that Berry is meant to be mostly backward compatible with Yarn (it uses the same workspace definition, for example, with less bugs), so I'd recommend giving it a try when you get the chance 🙂

I've been looking into it :) only one of our repository doesn't really require our private npm registries, so I threw away the yarn.lock, .yarnrc and .npmrc and gave it a go. I didn't get far, sadly, because of a lack of support for PnP in internal/external tooling. Berry for some reason also refuses to run the postinstall script of the main workspace. In any case stuff I need to look into when I find the time to do so

The main worry I'd have would be to complexify the two gen_ rules - hopefully DependencyType is the last parameter we'd need to add.

We could keep the old versions around, i.e. have

gen_invalid_dependency(WorkspaceCwd, DependencyName, DependencyRange).
gen_invalid_dependency(WorkspaceCwd, DependencyName, DependencyRange, DependencyType).

Although personally I wouldn't take that road, because it increases the complexity: we'd have four predicates to query and writers of constraints are more likely to get confused.

Another option might be to create a "dependency record":

% Require peerDependencies to also be present in devDependencies
gen_invalid_dependency(WorkspaceCwd, DependencyName, DependencyRecord):-
  dependency_type(DependencyRecord, devDependencies),
  workspace_has_dependency(WorkspaceCwd, DependencyName, FoundDependencyRecord),
  dependency_type(FoundDependencyRecord, peerDependencies),
  dependency_version(FoundDependencyRecord, VersionRange),
  dependency_version(DependencyRecord, VersionRange).

% Disallow tslib as dependency, peerDependency and devDependency
gen_invalid_dependency(WorkspaceCwd, 'tslib', DependencyRecord):-
  workspace_has_dependency(WorkspaceCwd, 'tslib', _),
  dependency_version(DependencyRecord, null).

That way, if you don't care about the type or the version, you don't have to include it anywhere. It doesn't make the prolog code shorter or easier to understand though. It also increases the number of predicates we'd need to provide.

workspaces aren't required to have names. Similarly, two workspaces may have the same name but different versions (I admit it's a bit of a fringe case, but it may happen).

Yarn v1 doesn't support multiple workspaces with the same name and it blatantly ignores workspaces without name in yarn workspaces info, so this didn't cross my mind. The path relative to the workspace root seems like the best identifier then.

[arcanis]

Berry for some reason also refuses to run the postinstall script of the main workspace.

Woops, fixed in #34! 😄

Regarding gen_invalid_dependency, I agree it's better to just keep one single version, at least for now. Prolog will already be a big enough step, I prefer not to add arity distinction at the same time. Records are an interesting idea, but may be a bit overkill - I don't think they would get additional fields in the future.

Yarn v1 doesn't support multiple workspaces with the same name

Well, I'm surprised - I sure thought it did! I completely forgot I added a check to protect against ... well, that changes things. Since this restriction has apparently never been a problem before, noone relies on it and I can discard it from Berry 🤔

[arcanis]

Fixed with #71! 👍