nrwl / nx

Smart Monorepos · Fast CI
https://nx.dev
MIT License
23.27k stars 2.32k forks source link

be able to depend on a configuration of a target #15064

Open dmitry-stepanenko opened 1 year ago

dmitry-stepanenko commented 1 year ago

Description

Right now dependsOn syntax allows to specify only the name of the target. However, there're certain scenarios where it would be beneficial to have an ability to depend on a certain configuration of this target

Motivation

Consider the following scenario (it is a bit simplified just to get an idea):

With this setup it is not possible to properly specify the dependsOn, because for deploy.custom I need to depend on myapp:build and for deploy.cloudflare I need to depend on myapp:build:cloudflare. Of course, there're several workarounds:

{
    "name": "myapp",
    "projectType": "application",
    "sourceRoot": "apps/myapp/src",
    "targets": {
      "build": {
        "executor": "my-executor",
        "options": {
          "configFile": "apps/myapp/config.ts"
        },
        "configurations": {
          "cloudflare": {
            "configFile": "apps/myapp/config.cloudflare.ts"
          }
        }
      },
      "deploy.custom": {
        "executor": "nx:run-commands",
        "options": {
          "command": "node scripts/deploy.js"
        },
        "dependsOn": ["build"]
      },
      "deploy.cloudflare": {
        "executor": "@k11r/nx-cloudflare-wrangler:deploy-page",
        "options": {
          "branch": "master",
          "projectName": "myapp",
          "dist": "dist/apps/myapp/client"
        },
        "dependsOn": ["build-cloudflare"]
      },
      "build-cloudflare": {
        "executor": "nx:run-commands",
        "options": {
          "command": "npx nx run myapp:build:cloudflare"
        }
      }
    }
  }

There're some additional arguments mentioned in this discussion https://github.com/nrwl/nx/discussions/9236

Suggested Implementation

An ideal scenario would be to specify a certain configuration explicitly in the dependsOn

      "deploy.cloudflare": {
        "executor": "@k11r/nx-cloudflare-wrangler:deploy-page",
        "options": {
          "branch": "master",
          "projectName": "myapp",
          "dist": "dist/apps/myapp/client"
        },
        "dependsOn": ["build:cloudflare"]
      },

I have even started implementing this, but reached a point where I guess it needs to be discussed.

The way I'm expecting this to work is:

The problem is that this approach creates certain ambiguity, because there's a chance that certain target is specified as a dependency of multiple other targets with different configurations. So that dependency map may look like this

{
  'app1:build': ['lib1:build:production', 'lib3:build:production'],
  'lib1:build:production': ['lib3:build:ci'],
  'lib3:build:ci': [],
  'lib3:build:production': [],
}

Workarounds

--nx-accept-same-target-variations flag

The first proposed approach is to present a new flag --nx-accept-same-target-variations, which is false by default.

Expected behavior with the scenario described above and w\o --nx-accept-same-target-variations flag: nx run app1:build throws:

>  NX   Could not execute command because the task graph has targets included more than once with different configurations

  Targets:
  lib3:build:ci, lib3:build:production

   Rerun the command with "--nxAcceptSameTargetVariations" flag if this behavior is intended.

Running nx run app1:build --nx-accept-same-target-variations shows a warning:

>  NX   The task graph has targets included more than once with different configurations

   Targets:
   lib3:build:ci, lib3:build:production

Target configuration param

As an alternative instead of expecting aflag that applies to all targets it might be better to have a newacceptSameTargetVariations` option on the target configuration that we are fine to run multiple times

      "do-stuff": {
        "executor": "my-executor",
        "acceptSameTargetVariations": true,
        "options": {
          "configFile": "apps/myapp/config.ts"
        }
      }

And in the same manner as in the approach above we can throw a validation error if "acceptSameTargetVariations": true is not present

>  NX   Could not execute command because the task graph has targets included more than once with different configurations

  Targets:
  lib3:build:ci, lib3:build:production

   If this is intended, set "acceptSameTargetVariations" option to true in target's configuration.
rlmestre commented 1 year ago

dependsOn has an expanded syntax (docs) that covers configuration forwarding and other params:

"build": {
   // forward params passed to this target to the dependency targets
  "dependsOn": [{ "projects": "dependencies", "target": "build", "params": "forward" }]
},

To run different executors based on config, you could keep them on separate targets and write another executor that handles that logic. It seems to me that being able to depend on a specific configuration implies maintaining multiple targets for each one anyway.

dmitry-stepanenko commented 1 year ago

@rlmestre please take a look at the example that I have in the issue description. It doesn't make much sense to maintain intermediate target build-cloudflare, which has an only purpose of changing the configuration build target is supposed to run with. Moreover, there're other reasonable use cases described here https://github.com/nrwl/nx/discussions/9236

robwilkes commented 1 year ago

Hmmm, I would like this as well, I have a build target for google-cloud-build and would always depend on app:build:production. It's a bit of a pain needing to pass --configuration=production to google-cloud-build every time I run that target and it risks somebody running it in future and forgetting to pass this flag. That said CI/CD will eventually make it a non-issue but in the meantime it seems like a step that could be improved.

Hmm, and if I set defaultConfiguration as production, it doesn't call app:build:production from buildTarget it just calls app:build. It only calls upstream app:build:production if I specify it as --configuration, this seems like a bug? If the defaultConfiguration is production and it has a buildTarget of app:build:production surely it should call that and not just app:build ??

dmitry-stepanenko commented 1 year ago

@AgentEnder @FrozenPandaz I'm thinking that it would be better to control whether target can be run multiple times on a target itself. I've updated the ticket description with the following:

As an alternative instead of expecting aflag that applies to all targets it might be better to have a newacceptSameTargetVariations` option on the target configuration that we are fine to run multiple times

      "do-stuff": {
        "executor": "my-executor",
        "acceptSameTargetVariations": true,
        "options": {
          "configFile": "apps/myapp/config.ts"
        }
      }

And in the same manner as in the approach above we can throw a validation error if "acceptSameTargetVariations": true is not present

>  NX   Could not execute command because the task graph has targets included more than once with different configurations

  Targets:
  lib3:build:ci, lib3:build:production

   If this is intended, set "acceptSameTargetVariations" option to true in target's configuration.

Let me know if this works better for you, I'll update my PR

tnrich commented 1 year ago

@dmitry-stepanenko thanks for taking this issue on. I'd love to see this solved in a clean and simple way. I was surprised when my

"dependsOn": ["build:demo"],

didn't just work out of the box. It definitely seems like the right approach to allow this sort of syntax.

@AgentEnder @FrozenPandaz can we please get this PR in? Thank you!!

Here's what I'm hoping to have work in the future:

image
dimitriy-k commented 1 year ago

I need it too!

"dependsOn": ["build:demo"],

lppedd commented 10 months ago

Extremely useful, and got here just two days after using Nx, so this should tell you something.
Bumping for visibility.

masonmark commented 9 months ago

Yeah, we've spent several man-hours working around this issue since we adopted Nx some years ago... it comes up on most projects that aren't simple libs, and our engineers have worked around it a dozen different ways by now, and our project.json files are littered with those different workarounds.

And like other commenters, pretty much everybody asked how to do this within the first week or two of being introduced to Nx. So I'd also like to keep this issue alive for the holidays, too.... 🎅

mattfysh commented 7 months ago

Can anyone from nx speak to the resistance against implementing this feature? Is there something we're doing wrong by mixing configuration names together in the task graph, eager to learn if there is a better approach that we should be taking here

bsafwen commented 7 months ago

my team decided to use Nx and we stumbled upon this issue.

ciriousjoker commented 7 months ago

If this was implemented, here's our usecase: We have an Angular prerender target, but Angular doesn't support localization with prerendering. This forces us to use one configuration of that prerender target for each language.

We'd like to do this:

"dependsOn": [
  "prerender:production-en",
  "prerender:production-de",
]
elbazon commented 5 months ago

Would love to see this feature as-well. Very usefull.

gabrielbryk commented 3 months ago

Can anyone from nx speak to the resistance against implementing this feature? Is there something we're doing wrong by mixing configuration names together in the task graph, eager to learn if there is a better approach that we should be taking here

This would be great. This seems like a huge oversight by not supporting this, all my builds are defaulting to their dev configuration and it's forcing me to do some weird workarounds, why not support this?

ntinkler-calendly commented 3 months ago

I'll add that this is a giant pain for us.

We make (read - made the mistake of) fairly extensive use of configurations, and right now it's incredibly painful to not be able to pick a configuration as a dependency.

The existing solutions are hacky - so much so that it's often easier (and frankly more sane) to just bump out to shell scripts with run-command and run the exact commands (and their specific configurations) we need, but this removes 95% of the utility of NX.

To be extra clear - we don't even fall into the edge cases presented here. We do not trigger runs that would attempt to build multiple configurations of a target. But I need the ability to specify a target and a configuration by name in the deps array. The configuration names are not always the same across targets either, so forwarding args is nowhere close to getting us where we'd like to be.


After stewing on this pain for a year+... My stance is now this:

I don't think configurations should exist if you can't specify them as dependencies. What's the point of being able to partially override my target config if I can't reliably do it in all places that need it...?

It seems like it would have been far better to just force us to copy the entire target, instead of pretending this case is supported. Sadly - this makes the configuration of projects much more verbose and much more prone to code-rot and copy-paste errors.

lppedd commented 2 months ago

Eight months after my initial comment on this issue, and my Nx configuration files have become a total mess, I have to spend hours figuring out which duplicated targets to change each time.

This is making me want to get rid of Nx and switch to setting up Gradle tasks, since I'm already using it for Kotlin projects.
And if I'm at the point of considering Gradle... something enormously wrong is happening.

Lp-Francois commented 2 months ago

Hey! We do have the same issue where most of our docker commands need to depends on build:production, but we need to slightly change some params like load: true, push: true and the tags.

Right now it makes everything super verbose and duplicated :(

We tried to improve this with moving everything we can in nx.json as a set of default options for docker targets, that makes it slightly less verbose.

hinogi commented 2 months ago

@dmitry-stepanenko thanks for taking this issue on. I'd love to see this solved in a clean and simple way. I was surprised when my

"dependsOn": ["build:demo"],

didn't just work out of the box. It definitely seems like the right approach to allow this sort of syntax.

@AgentEnder @FrozenPandaz can we please get this PR in? Thank you!!

Here's what I'm hoping to have work in the future: image

I mean, you could make the dirty trick like

{
  "build": {
    ...
    "configuration": {
      ...
    },
  "build:demo": {
    ...
    "configuration": {
      ...
    }
}

it at least looks like it would use the config but you just have to boilerplate build several times just for a different config

AgentEnder commented 1 month ago

Hey - its been a long time since we talked about this as a core team but @FrozenPandaz and I recently sat some time aside to discuss it.

TLDR

We are going to close the linked PR for now. Its been open too long and we don't want to further string @dmitry-stepanenko along while we try to figure out what the correct path forward is here. His effort is greatly appreciated. We know this is going to be a bit controversial, but we are trying to make the best Nx Nx can be and believe that there is more thought necessary here.

Why are we closing the linked PR?

While we know as a team that this is a highly requested feature and we appreciate community contributions the PR has been open for too long without response from us. This is not something we take pride in. That said, combined with the fact that significant changes would be necessary and those changes are hard to express as there are still a ton of unknowns, we believe it would be better for this change to come from someone on the core team after a significant amount of design time has been spent thinking through the problem and edge cases.

Where do we go from here?

Ok, so if we still think this is a good feature and something we want to have we need to think through some edge cases while also thinking through the initial problem statement. I'm sure everyone here has a valid use case for this feature given the current state of Nx, but maybe there could even be a different change that fits the use case better.

As an example, if targets could have an extends property to extend another target on the same project, that may be enough to satisfy some use cases while keeping things distinct targets.

But, lets continue down the rabbit hole of allowing configurations to be depended on directly rather than another larger change. This sounds simple, and at the base level implementation isn't obscene, but there are some concerns and thus edge cases.

A few of our chief concerns with just allowing this is as follows:

I'm including a few edge cases below as a sample of what we are thinking about.

Localization based configurations

Imagine you have 2 projects, "french-site" and "english-site" that depend on a buildable shared library. That buildable shared library has a target, build, with two configurations ("en, and fr"). The french site's build target has depends on configured as: [{ target: build, dependencies: true, configuration: fr}]. The english site has a similar configuration.

graph TD
    B[french-site] -->|"build (fr)"| A[Shared Library]
     C[english-site] -->|"build (en)"| A[Shared Library]

The user runs nx run-many -t build. What tasks should Nx run, and in what order? The expanded task graph would look something like this naively:

graph TD
    B[french-site:build] --> A["shared-library:(build:fr)"]
    C[english-site:build] --> D["shared-library:(build:en)"]
    E["shared-library:build"]

Immediately there is likely to be a problem. Nx will say "I can run shared-library:build, shared-library:build:fr, and shared-library:build:en in parallel, as they all need to be ran and have no dependencies". The outputs of the targets will likely all be written on top of each other leaving the filesystem in a nondeterministic state that isn't likely to produce the desired outputs when the french and english sites build. If things are ran in serial, its even worse as the 3 tasks with no deps will run first, leaving whichever ran last's artifacts to be consumed by both the french and english site.

So we try to avoid that. We can either:

Separate targets makes the most sense to us. This is the existing workflow that many in this thread have already adopted, but there is a pain point in that target configurations allow pulling common options to the top level options block rather than duplicating things. This is where we'd potentially propose the addition of an "extends" property inside of a target declaration.

Linear Dependency Graph

Imagine you have 3 projects, a b and c. Each have a build target with dev and prod configurations. A depends on B and B on C. A's build target has a dependsOn block defined like: [{dependencies: true, target: build, configuration: prod}]. B's dependsOn configuration doesn't specify a specific configuration.

graph TD
    A -->|"build (prod)"| B
     B -->|build| C

When the user runs nx run-many -t build, what should happen? The initial task graph after some deduplication would look something like this:

graph TD
 A[a:build] --> B[b:build:prod] --> C[c:build:?]
 D[b:build] --> E[c:build]

Semantically, we believe that if the dependency between B and C doesn't specify a configuration, it should inherit the configuration from the parent task. So, since B was built prod due to the dependency on A, in the first chain at least, C would build with prod as well.

The second chain is arguably a duplicate execution path that wouldn't be needed, but this may not be the case if for whatever reason the target would fail in one configuration but not its default or similar.

Conclusion

We understand that this decision might not be ideal for everyone, but we believe it is necessary to ensure the best possible outcome for Nx. We deeply value the community's contributions and encourage continued dialogue and collaboration. Our goal is to revisit this feature with a well-thought-out approach that addresses the complexities and edge cases we've identified. Thank you for your understanding and continued support as we strive to improve Nx together.

gcko commented 1 week ago

If you need to call a specific configuration rather than the default, you can work around this issue today by flattening some of the configurations you need into their own distinct task runner. TO BE CLEAR this is absolutely not ideal, but in some cases this may solve your needs.

Example:

You can take the following test configuration:

{
"test": {
      "executor": "@nx/jest:jest",
      "outputs": ["{workspaceRoot}/coverage/{projectRoot}"],
      "defaultConfiguration": "development",
      "options": {
        "jestConfig": "apps/demo/jest.config.ts"
      },
      "configurations": {
        "development": {
          "watchAll": false,
          "coverageReporters": [
            "text-summary"
          ]
        },
        "pre-commit": {
          "ci": false,
          "bail": 1,
          "watchAll": false,
          "codeCoverage": false
        },
        "ci": {
          "testResultsProcessor": "jest-junit",
          "updateSnapshot": true,
          "ci": true,
          "watchAll": false,
          "codeCoverage": true
        }
      }
    },
}

And flatten it into multiple tasks:

...
"test-pre-commit": {
      "executor": "@nx/jest:jest",
      "outputs": ["{workspaceRoot}/coverage/{projectRoot}"],
      "options": {
        "jestConfig": "apps/ts/jest.config.ts",
        "ci": false,
        "bail": 1,
        "watchAll": false,
        "codeCoverage": false
      }
    },
    "test-ci": {
      "executor": "@nx/jest:jest",
      "outputs": ["{workspaceRoot}/coverage/{projectRoot}"],
      "options": {
        "testResultsProcessor": "jest-junit",
        "updateSnapshot": true,
        "ci": true,
        "watchAll": false,
        "codeCoverage": true
      }
    },
...

Again, this only addresses certain use-cases, but it may be helpful for some of you.