nrwl / nx

Smart Monorepos Β· Fast CI
https://nx.dev
MIT License
23.56k stars 2.36k forks source link

@nrwl/nx/enforce-module-boundaries allow AND/OR #6747

Closed webberwang closed 3 years ago

webberwang commented 3 years ago

Description

Allow AND/OR usage with tags.

Motivation

Currently the behavior is OR only, having AND would enable many more combinations to be possible without introducing new tags.

Suggested Implementation

Not so much implementation but an example api

"depConstraints": [
  {
    "sourceTag": "layer:ui",
    "onlyDependOnLibsWithTags": [{
      "tags": ["scope:admin", "layer:backend"],
      "operation": "AND"
    }]
  },
]
meeroslav commented 3 years ago

You don't need to introduce the new tag or have new operator. You can simply split it into two constraints. The following behaves as AND:

"depConstraints": [
  {
    "sourceTag": "layer:ui",
    "onlyDependOnLibsWithTags": ["scope:admin"]
  },
  {
    "sourceTag": "layer:ui",
    "onlyDependOnLibsWithTags": ["layer:backend"]
  },
]

It does look like a bit of overhead, but the benefit is it gives you granular control and an overview of how tags are connected. Let me know if this works for your use case.

Note: The following logic is not implemented The other variant I see could be (notice inner array acting as AND):

"depConstraints": [
  {
    "sourceTag": "layer:ui",
    "onlyDependOnLibsWithTags": [["these:are", "joined:with:and"], "this:one:joins:with:or"]
  },
]

But I'm not convinced this wouldn't end up being the foot gun.

github-actions[bot] commented 3 years ago

This issue has been automatically marked as stale because it hasn't had any recent activity. It will be closed in 14 days if no further activity occurs. If we missed this issue please reply to keep it active. Thanks for being a part of the Nx community! πŸ™

mperktold commented 3 years ago

I have a similar issue, but more related to AND vs OR of the tags associated to a project.

I have a workspace with multiple apps that belong to different groups. Some of them use a framework called framework that is implemented as a lib, some don't. I tried to capture which projects can use framework and which can't using the following configuration.

nx.json:

"projects": {
     "app-a1": {
          "tags": ["group-a", "framework"]
      },
      "app-a2": {
          "tags": ["group-a"]
      },
     "app-b1": {
          "tags": ["group-b", "framework"]
      },
      "app-b2": {
          "tags": ["group-b"]
      },
      "lib-a": {
          "tags": ["group-a"]
      },
      "lib-b": {
          "tags": ["group-b"]
      },
      "framework": {
          "tags": ["framework"]
      },
      "shared": {
          "tags": ["shared"]
      }
}

.eslintrc.json

"depConstraints": [
    {
        "sourceTag": "shared",
        "onlyDependOnLibsWithTags": ["shared"]
    },
    {
        "sourceTag": "framework",
        "onlyDependOnLibsWithTags": ["shared", "framework"]
    },
    {
        "sourceTag": "group-a",
        "onlyDependOnLibsWithTags": ["shared", "group-a"]
    },
    {
        "sourceTag": "group-b",
        "onlyDependOnLibsWithTags": ["shared", "group-b"]
    }
]

So basically, I divide apps and libs into "group-a" and "group-b". Independently of that, there is the framework, and I add the "framework" tag to all apps (and possibly also libs) that are allowed to depend on it, in the hope that a project with two or more tags can depend on the union of allowed dependencies as per .eslintrc.json.

This doesn't work, as a project with more than one tag seems to be allowed to depend only on the intersection of the allowed dependencies listed in .eslintrc.json.

I find that counter-intuitive, but I admit that the name onlyDependOnLibsWithTags indeed suggests that it really is the intersection rather than the union. Also, the original post is an example where the current behavior is needed, as @meeroslav shows.

But what would the right approach to model my scenario currently look like? Introduce separate tags "group-a-with-framework" and "group-b-with-framework" that can depend everything the corresponding group can plus "framework"?

meeroslav commented 3 years ago

Thanks @mperktold, your problem actually got me thinking a bit.

Let's distill your requirements:

If we look at your depConstraints:

If you would mark framework also as shared (which it actually is), then following your constraints, also App A2 and B2 would be able to load it.

Your solution should work:

Unfortunately, the more complex your setup is, the more work you need to invest in setting the tags properly.

Meanwhile, I'll think about cleaner solution to your problem.

webberwang commented 3 years ago

All the proposed solutions are a workaround for the lack of AND/OR (or some other solution), and doesn’t scale that well.

meeroslav commented 3 years ago

Meanwhile, I'll think about a cleaner solution to your problem.

Please have in mind that your problems/solutions are contradicting. So we need to find a solution that would work in both cases and would still be intuitive.

mperktold commented 3 years ago

@meeroslav yes, the solution works and I ended up using it.

To give more food for thought, maybe instead of exposing some AND/OR logic, you could also provide another option, e.g. canDependOnLibsWithTags, parallel to onlyDependOnLibsWithTags, which would 'grant' the right to depend on the given other tags. When a project has multiple tags, each of which has only restrictions of type canDependOnLibsWithTags, then it can depend on the union of them all.

The question is then what should happen if both kinds of restrictions apply to a single project. I would say that onlyDependOnLibsWithTags should be stronger than canDependOnLibsWithTags, meaning that it can 'remove' tags from the list of allowed dependency tags, even if it would be granted by canDependOnLibsWithTags.

But I don't want to dictate a solution, just explore some ideas.

meeroslav commented 3 years ago

@mperktold glad to hear that the solution works (not that I suspected it wouldn't πŸ˜€)

Thank you for the proposal. You are correct thatonlyDependOnLibsWithTags is too rigid and makes the corner cases like yours too complicated to implement. We still want the dependency constraint rule to target a single aspect of the app, otherwise, we can just as well use the group-x-framework approach. Not sure if this would make sense:

"depConstraints": [
    {
        "sourceTag": "shared",
        "onlyDependOnLibsWithTags": ["shared"]
    },
    {
        "sourceTag": "framework",
        "onlyDependOnLibsWithTags": ["shared", "framework"]
    },
    {
        "sourceTag": "group-a",
        "onlyDependOnLibsWithTags": ["shared", "group-a"],
        "canDependOnLibsWithTags": ["framework"]
    },
    {
        "sourceTag": "group-b",
        "onlyDependOnLibsWithTags": ["shared", "group-b"],
        "canDependOnLibsWithTags": ["framework"]
    }
]

The rule would not break even if app-a1 would not have the framework tag, which is not what we want. Probably the current approach where we compare TAG X with TAG Y agnostic of other source node's tags is not enough.

mperktold commented 3 years ago

You are correct thatonlyDependOnLibsWithTags is too rigid and makes the corner cases like yours too complicated to implement.

Actually I wouldn't say it's too compliated in this case. Beeing able to compose existing tags to get the desired behavior would be nice, but I'm also fine with introducing new tags in my case.

I can immagine that other setups are more complicated and would profit even more from such a feature, but I'm not sure whether that's really the case.

With something like canDependOnLibsWithTags implemented, I would probably use it in this way:

"depConstraints": [
    {
        "sourceTag": "shared",
        "onlyDependOnLibsWithTags": ["shared"]
    },
    {
        "sourceTag": "framework",
        "canDependOnLibsWithTags": ["shared", "framework"]
    },
    {
        "sourceTag": "group-a",
        "canDependOnLibsWithTags": ["shared", "group-a"]
    },
    {
        "sourceTag": "group-b",
        "canDependOnLibsWithTags": ["shared", "group-b"]
    }
]

Everything can depend on shared, but anything tagged with shared should really only depend on shared, since it should not pull unwanted dependencies into other projects.

The tags group-a and group-b can be given to a project to allow it to depend on other projects of that group. This is independent of framework, it neither allows nor forbids to depend on that.

To allow to depend on the framework, a project must have the framework tag. This allows us to control whether an app can depend on the framework or not, even if it is part of a group. I think that is a key difference to your proposal.

meeroslav commented 3 years ago

Actually, the reason why it was behaving unexpected was that you are mixing two different dimensions - scope (group-a, group-b, shared) with technical implementation (framework).

This should work:

nx.json:

"projects": {
     "app-a1": {
          "tags": ["group-a", "framework"]
      },
      "app-a2": {
          "tags": ["group-a", "non-framework"]
      },
     "app-b1": {
          "tags": ["group-b", "framework"]
      },
      "app-b2": {
          "tags": ["group-b", "non-framework"]
      },
      "lib-a": {
          "tags": ["group-a", "non-framework"]
      },
      "lib-b": {
          "tags": ["group-b", "non-framework"]
      },
      "framework": {
          "tags": ["shared", "framework"]
      },
      "shared": {
          "tags": ["shared", "non-framework"]
      }
}

.eslintrc.json

"depConstraints": [
    {
        "sourceTag": "shared",
        "onlyDependOnLibsWithTags": ["shared"]
    },
    {
        "sourceTag": "group-a",
        "onlyDependOnLibsWithTags": ["shared", "group-a"]
    },
    {
        "sourceTag": "group-b",
        "onlyDependOnLibsWithTags": ["shared", "group-b"]
    },
    {
        "sourceTag": "framework",
        "onlyDependOnLibsWithTags": ["framework", , "non-framework"]
    },
    {
        "sourceTag": "non-framework",
        "onlyDependOnLibsWithTags": ["non-framework"]
    }
]

What we did here:

  1. framework lib is shared (used by both group-a and group-b, so it should be marked accordingly)
  2. This allows then non-framework app-a2 or lib-a to use framework so we need to restrict this
  3. First we make sure framework projects can depend on other framework but also on non-framework projects
  4. Finally, non-framework cannot depend on framework

You don't need to introduce new combo tags e.g. group-a-framework. You just need to make sure you cover entire vertical. It's often easier to have prefixes, which helps to identify missing tags:

mperktold commented 3 years ago

That makes sense.

What I find a bit awkward in this solution is that I must explicitly assign non-framework tags to low-level libs that neither know nor care whether there is a framework.

Still, it's a solid approach, and seems to be more consistent with nx's philosophy then my version. I might switch to this one, thanks!

meeroslav commented 3 years ago

I used non-framework for simplicity. In real application, you would never use such tag name. You could use architecture:x or something in those lines. But I hope you got the idea - if you bring new dimension (support for framework in your case) to the equation you need to make sure all the projects have an opinion (tag) on how they want to handle it, otherwise your rules will ban them.

We usually use scope:x for focus (vertical) segmentation and type:x for horizontal segmentation (apps, ui-features, libs, utils etc.). Usually all projects within one scope use the same framework, so there is no need for an explicit framework tag.

github-actions[bot] commented 1 year ago

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.