feat(linter): add notDependOnLibsWithTags constraint option to enforce-module-boundaries rule

[jaytavares]

Current Behavior

onlyDependOnLibsWithTags is the only option currently available for setting up tag-based constraints. See #984 for discussion of some limitations this presents

Expected Behavior

Addition of a notDependOnLibsWithTags option; provides the ability to prevent the import of a project that contains a disallowed tag.

Note: The nature of this option is a bit different than onlyDependOnLibsWithTags in that we really want to make sure a library tagged with a disallowed tag doesn't find its way into the source project. For this reason, I coded the implementation with a recursive search for the disallowed tags on all dependent projects.

Once I get some confirmation on the above approach, I can finish the remaining tasks:

TODO:

• Add tslint implementation
• Add project dependency path to report message (similar to circular dependency check message)
• Document new constraint option

Related Issue(s)

Fixes #984

[nx-cloud]

☁️ Nx Cloud Report

CI ran the following commands for commit 9fa9ec5. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 6 targets

• nx affected --target=build --base=1390cd086c5bfad61868d252691fd04468c9bcca --head=9fa9ec56f1c0e96d36a92ea5eb19be4c397b32ad --parallel=3
• nx affected --target=e2e --base=1390cd086c5bfad61868d252691fd04468c9bcca --head=9fa9ec56f1c0e96d36a92ea5eb19be4c397b32ad --exclude=e2e-detox,e2e-js,e2e-next,e2e-workspace-create,e2e-workspace-integrations,e2e-workspace-core,e2e-react,e2e-web,e2e-angular-extensions,e2e-angular-core,e2e-cli,e2e-nx-plugin,e2e-cypress,e2e-node,e2e-linter,e2e-jest,e2e-a...
• nx affected --target=e2e --base=1390cd086c5bfad61868d252691fd04468c9bcca --head=9fa9ec56f1c0e96d36a92ea5eb19be4c397b32ad --exclude=e2e-storybook,e2e-react-native,e2e-detox --parallel=1
• nx affected --target=lint --base=1390cd086c5bfad61868d252691fd04468c9bcca --head=9fa9ec56f1c0e96d36a92ea5eb19be4c397b32ad --parallel=3
• nx affected --target=test --base=1390cd086c5bfad61868d252691fd04468c9bcca --head=9fa9ec56f1c0e96d36a92ea5eb19be4c397b32ad --parallel=1
• nx build typedoc-theme

---

Sent with 💌 from NxCloud.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nrwl/nx-dev/B7shm71yXh8gJHUaMcvcUJnRx5kA
✅ Preview: https://nx-dev-git-fork-curiovision-implement-not-depend-on-3b9843-nrwl.vercel.app

[vsavkin]

@meeroslav can you take a look?

We need to make sure that:

1. We don't introduce O(N^N) where N is the number of projects. Workspaces that have thousands of projects tend to have problems with it. We might want to do an extra traversal where we all the tags for all the projects, which will be O(N). Then nothing recursive will be required.
2. Make sure it's documented.

[jaytavares]

Fyi, I think failed test is leaking in from upstream code. I can't find a cause locally in this PR.

[meeroslav]

@jaytavares thank you for your PR.

I think the functionality makes sense, but there are a few things I would change:

• Only one of onlyDependOnLibsWithTags and notDependOnLibsWithTags should exists at the time in the config. We need a strict check to make sure they are not both defined.
• I don't think recursion is needed (nor look into depth). We should instead make sure that we are consistent in the restrictions. Look at the example below:

Let's say you have the following graph:

LIB A(`tag-Angular`) -\
                       \	
	                 LIB B(`tag-Shared`) ---- LIB C(`tag-React-Util`)
LIB D(`tag-React`) --- /

Let's say you want to set up the rule that tag-Angular cannot depend on the tag-React-Util, but tag-React can.
That would mean also that tag-Shared cannot depend on tag-React-Util otherwise tag-Angular wouldn't be able to depend on the tag-Shared.
In which case you would have to specify that tag-Shared also cannot depend on tag-React-Util and that makes the recursion unnecessary.

[meeroslav]

Fyi, I think failed test is leaking in from upstream code. I can't find a cause locally in this PR.

Yeah, try rebasing on to the latest master. That should solve those E2E tests

[jaytavares]

• Only one of onlyDependOnLibsWithTags and notDependOnLibsWithTags should exists at the time in the config. We need a strict check to make sure they are not both defined.

Are you thinking that we'd require separate config entries?

Like this:

"depConstraints": [
  {
    "sourceTag": "scope:tag-A",
    "onlyDependOnLibsWithTags": ["scope:tag-A"]
  },
  {
    "sourceTag": "scope:tag-A",
    "notDependOnLibsWithTags": [
      "scope:tag-B",
    ]
  }
]

Rather than a combined syntax like this:

"depConstraints": [
  {
    "sourceTag": "scope:tag-A",
    "onlyDependOnLibsWithTags": ["scope:tag-A"]
    "notDependOnLibsWithTags": [
      "scope:tag-B",
    ]
  }
]

[meeroslav]

Are you thinking that we'd require separate config entries?

What I meant was disallowing both. The rule should throw an error if it finds the same sourceTag defined with onlyDependOnLibsWithTags and notDependOnLibsWithTags regardless of whether it's in the same constraint block or in separate ones.

If it's ok with you I can add this code to your PR.

[jaytavares]

Hmm. Why do you think it's necessary to limit libraries to only one or the other? It seems like it would be more versatile to allow both.

[meeroslav]

I have a few minor changes. Also, don't forget to add the new property to documentation on https://github.com/nrwl/nx/blob/master/docs/shared/monorepo-tags.md.

If you want, you can skip the documentation and I will add it after your PR is merged.

Ignore the tslint as that linter is deprecated so we are not supporting it with new features anymore, only bug fixes.

[meeroslav]

Remove the recursion all look only at the first level of dependencies. We can always add matrix graphs, similar to circular dependencies if there is ever a need.

[jaytavares]

(More discussion needed, see PR conversation thread)

[meeroslav]

Why not targetProject like on line 397?

[jaytavares]

Because I don't have a full ProjectGraphNode object within the recursive call here.

If this call goes away, we can and should change this to match the similar function signature on line 397. I think there's still some discussion to be had regarding this though. (See PR conversation thread)

[meeroslav]

Hmm. Why do you think it's necessary to limit libraries to only one or the other? It seems like it would be more versatile to allow both.

Yes, sorry, I was thinking in a single dimension. With multiple tags per project, it makes sense to have both at the same time. Thanks for questioning my proposal.
My concern was that tags are already a too complex topic for some folks and adding new potentially overlapping property will only make it worse.

[jaytavares]

Let's say you have the following graph:

LIB A(`tag-Angular`) -\
                       \	
	                 LIB B(`tag-Shared`) ---- LIB C(`tag-React-Util`)
LIB D(`tag-React`) --- /

Let's say you want to set up the rule that tag-Angular cannot depend on the tag-React-Util, but tag-React can. That would mean also that tag-Shared cannot depend on tag-React-Util otherwise tag-Angular wouldn't be able to depend on the tag-Shared. In which case you would have to specify that tag-Shared also cannot depend on tag-React-Util and that makes the recursion unnecessary.

Hmm, I get what you're saying; however, I feel like this behavior would be less intuitive. (see below)

We should instead make sure that we are consistent in the restrictions.

With onlyDependOnLibsWithTags, the constraint will only pass if the given tag is added to the target lib. The presence of the tag on the target lib explicitly communicates that the library is suitable for use for the purposes of the tag. This is a failsafe condition.

The opposite is true for notDependOnLibsWithTags. It will only fail if the target has the given tag. If the author of the target library didn't realize they needed to apply a tag to their library simply because they imported from a problematic library, there will be no protection offered to consumers of their library. This is the opposite of failsafe.

It really does seem that we need deeper inspection for the latter since we lack the explicitness of the former. As for how we go about that inspection, I'm totally open to suggestions. I modeled my recursive function on the getPath() function used by the circular dependency check; however, I left the matrix stuff out since it was lacking comments explaining its purpose. I get the reasoning now. If you want, I can take a stab at the matrix approach.

[meeroslav]

The tooling should not compensate for the lack of good hygiene (setting proper tags wherever needed) with running extra checks. If the linter rule is slow, it will drive people to switch it off. That's why recursion is not an option.

The whole point of the matrix is to create structure once and keep it in memory, so we don't have to use recursion every time. Finding a path is then a straightforward process. This part of the linter rule is currently the most problematic one (see #8313) and will be redesigned soon.

Therefore my suggestion is to finish the PR without using the matrix (or any other in-depth checks). We can add this as an improvement with a follow-up PR.

Otherwise, feel free to look at the implementation of the matrix and try to re-use it.

[meeroslav]

Hey @jaytavares, just checking in to see if you need help with finishing this PR?

The issue with the rule causing the mismatches in IDE is merged in, so you have a clear path to work on this.

I can add the function to search for descendants with the given tag using the getPath function.

[jaytavares]

Hey @jaytavares, just checking in to see if you need help with finishing this PR?

The issue with the rule causing the mismatches in IDE is merged in, so you have a clear path to work on this.

I can add the function to search for descendants with the given tag using the getPath function.

Hey @meeroslav, sorry for the delayed response. I've been busy working on a work project. If you want to implement the search functionality using getPath() go for it! Otherwise, I can circle back to this once I've finished up my stuff in a week or two.

[meeroslav]

@jaytavares, is it ok if I continue working on your branch?

Once this is merged in you would still be marked as author of the PR/commit.

[jaytavares]

@jaytavares, is it ok if I continue working on your branch?

Once this is merged in you would still be marked as author of the PR/commit.

Sure, go for it.

[meeroslav]

Please grant me access rights to your repo so I can rebase and force push master to this branch.

[jaytavares]

Please grant me access rights to your repo so I can rebase and force push master to this branch.

Done

[github-actions]

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.