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

dependencyOf - the inverse of dependsOn #19414

Closed
1 task done
fxposter opened this issue Oct 2, 2023 · 14 comments · Fixed by #19611
Closed
1 task done

dependencyOf - the inverse of dependsOn #19414

fxposter opened this issue Oct 2, 2023 · 14 comments · Fixed by #19611
Labels

Comments

@fxposter
Copy link
Contributor

fxposter commented Oct 2, 2023

Description

I'd like to add the dependencyOf field to the TargetConfiguration interface, which would allow users and plugins to add additional targets that would automatically add themselves to other targets' dependsOn lists.

Motivation

Main motivation is to programmatically (in plugins) add targets that would execute before other targets. This would allow to easily implement preBuild functionality without needed to modify the existing build target or even know if it exists.

Suggested Implementation

The idea is to add a dependencyOf field to the TargetConfiguration and transform such dependencies to the dependsOn ones on the ProjectGraph creation step, so that everything that works with the ProjectGraph itself would not care about the dependencyOf and continue to rely only on dependsOn. It can be done after building full projectGraph here: https://github.com/nrwl/nx/blob/master/packages/nx/src/project-graph/build-project-graph.ts#L196 by iterating over all projects like this:

for (const project in finalGraph.nodes) {
  const targets = finalGraph.nodes[project].targets;
  if (targets) {
    for (const dependencyName in targets) {
      if (targets[dependencyName].dependencyOf) {
        for (const name of targets[dependencyName].dependencyOf) {
          if (targets[name]) {
            targets[name].dependsOn || (targets[name].dependsOn = []);
            targets[name].dependsOn.push(dependencyName);
          }
        }
        delete(targets[dependencyName].dependencyOf)
      }
    }
  }
}

Alternate Implementations

...

@fxposter
Copy link
Contributor Author

fxposter commented Oct 2, 2023

the mentioned implementation would transform these targets

{
  'a': {},
  'b': {
    dependsOn: ['a'],
    dependencyOf: ['c'],
  }
  'c': {
    dependsOn: ['d'],
  },
  'd': {
  }
}

to

{
  'a': {},
  'b': {
    dependsOn: ['a'],
  }
  'c': {
    dependsOn: ['d', 'b'],
  },
  'd': {
  }
}

maybe it would be a useful feature to copy original dependsOn from the c target to b as well, so that in the end we get this targets instead:

{
  'a': {},
  'b': {
    dependsOn: ['a', 'd'],
  }
  'c': {
    dependsOn: ['d', 'b'],
  },
  'd': {
  }
}

@fxposter
Copy link
Contributor Author

fxposter commented Oct 6, 2023

@AgentEnder @FrozenPandaz I see that you both work on project graph. Can you comment on whether it might be a good idea and PR for it might be merged or it would definitely be rejected?

@AgentEnder AgentEnder added the scope: core core nx functionality label Oct 6, 2023
@AgentEnder
Copy link
Member

Hey, I wouldn't recommend submitting a PR for this just yet. Its not a crazy feature or anything, but its something we will need to discuss internally to decide if its something that we want to support. Anything that could be described by dependencyOf, could after all be described by one or more dependendsOn targets too.

@fxposter
Copy link
Contributor Author

fxposter commented Oct 12, 2023

Maybe I am missing something, but how people handle cases where they simply want to split a target into 2 targets to run them in parallel?

ie: if I have setup, build and test targets, and build depends on setup, test depends on build.
if I want to split build in (build-css+build-js) and split test into (test-unit+test-e2e), that all can be run in parallel - on each such change - I'd need to change dependsOn lists on a lot of tasks, for example adding separate test-integration - I need to make it depend on (build-css+build-js) now.

With dependencyOf feature it can be modeled like this:
originally I have

{
  "targets": {
    "setup": {},
    "build": {
      "dependsOn": ["setup"],
    },
    "test": {
      "dependsOn": ["build"],
    }
  }
}

then I extract "build-css" into separate task:

{
  "targets": {
    "setup": {},
    "build": { // do build without css
      "dependsOn": ["setup"],
    },
    "build-css": {
      "dependencyOf": [{ "target": "build", "copyDependencies": true }],
    },
    "test": {
      "dependsOn": ["build"],
    }
  }
}

then I make "build" a dummy target, that does nothing and extract everything into separate tasks as well:

{
  "targets": {
    "setup": {},
    "build": { // do nothing
      "dependsOn": ["setup"],
    },
    "build-css": {
      "dependencyOf": [{ "target": "build", "copyDependencies": true }],
    },
    "build-js": {
      "dependencyOf": [{ "target": "build", "copyDependencies": true }],
    },
    "build-fonts": {
      "dependencyOf": [{ "target": "build", "copyDependencies": true }],
    },
    "test": {
      "dependsOn": ["build"],
    }
  }
}

then I can do the same trick with tests:

{
  "targets": {
    "setup": {},
    "build": { // do nothing
      "dependsOn": ["setup"],
    },
    "build-css": {
      "dependencyOf": [{ "target": "build", "copyDependencies": true }],
    },
    "build-js": {
      "dependencyOf": [{ "target": "build", "copyDependencies": true }],
    },
    "build-fonts": {
      "dependencyOf": [{ "target": "build", "copyDependencies": true }],
    },
    "test": { // do nothing
      "dependsOn": ["build"],
    },
    "test-unit": {
      "dependencyOf": [{ "target": "test", "copyDependencies": true }],
    },
    "test-e2e": {
      "dependencyOf": [{ "target": "test", "copyDependencies": true }],
    },
}

with such configuration the last target graph would be translated into this one:

{
  "targets": {
    "setup": {},
    "build": { // do nothing
      "dependsOn": ["setup", "build-css", "build-js", "build-fonts"],
    },
    "build-css": {
      "dependsOn": ["setup"]
    },
    "build-js": {
      "dependsOn": ["setup"]
    },
    "build-fonts": {
      "dependsOn": ["setup"]
    },
    "test": { // do nothing
      "dependsOn": ["build", "test-unit", "test-e2e"],
    },
    "test-unit": {
      "dependsOn": ["build"]
    },
    "test-e2e": {
      "dependsOn": ["build"]
    },
}

@AgentEnder
Copy link
Member

Yeah, so you just shown that the proposed dependencyOf syntax could easily be converted into dependsOn blocks. I don't see a ton of value in supporting both.

@fxposter
Copy link
Contributor Author

it's all about what can be added by developers without needing to rewrite their existing targets

@fxposter
Copy link
Contributor Author

currently if you have a chain of A target depends on B, B depends on C, if you need to insert something in the B layer (imagine the B is the "build" in my example) - you need to rewrite both B and C. with my proposal you can insert B1 without touching neither B, nor C.

@fxposter
Copy link
Contributor Author

the alternative that potentially can allow doing something like this is to allow dependOn to have wildcards, so that if I define

{
  "targets": {
    "build": {
      "dependsOn": ["^build"]
    },
    "test": {
      "dependsOn": ["build*"]
    }
  }
}

then I can split "build" to "build-css"+"build-js" without changing that test target. And I can define such additional targets in different plugins, for example.

@fxposter
Copy link
Contributor Author

@AgentEnder would wildcard/regex target dependencies have more chances to be accepted?

@fxposter
Copy link
Contributor Author

We're trying to build a CI system with multiple layers of task executions and nx in this case is lacking ability for plugins to add targets dynamically between already defined targets, cause V2 extension points don't allow you to see the "already defined targets for a project". Since v2 plugins were introduced for a reason - I understand that "modifying an already built project graph" is not something nx is willing to support - hence I am trying to find an extension points that could be accepted within "the nx vision".

@fxposter
Copy link
Contributor Author

I've made an example implementation for wildcards in target dependencies to make it clear what we would like to have.

@fxposter
Copy link
Contributor Author

@AgentEnder any chance to get at least preliminary response of the PR, please?

@fxposter
Copy link
Contributor Author

Closing in favor of #19611, but unfortunately it doesn't seem to be interested anyone from core team :(

Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 20, 2024
FrozenPandaz pushed a commit that referenced this issue Jul 8, 2024
Now it is possible to define targets like this:

```
{
  "targets": {
    "build-css": {},
    "build-js": {},
    "test": {
      "dependsOn": ["build-*"]
    },
  }
}
```

<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

## Current Behavior
No support for wildcard target dependencies.

## Expected Behavior
This PR is an example of what I described here:
#19414.

## Related Issue(s)
Closes #19414

---------

Co-authored-by: Craigory Coppola <[email protected]>
FrozenPandaz pushed a commit that referenced this issue Jul 8, 2024
Now it is possible to define targets like this:

```
{
  "targets": {
    "build-css": {},
    "build-js": {},
    "test": {
      "dependsOn": ["build-*"]
    },
  }
}
```

<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

## Current Behavior
No support for wildcard target dependencies.

## Expected Behavior
This PR is an example of what I described here:
#19414.

## Related Issue(s)
Closes #19414

---------

Co-authored-by: Craigory Coppola <[email protected]>
(cherry picked from commit 3e0d2de)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants