nrwl / nx

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

nx-release-publish default target dependsOn is ignored #27749

Closed ThePlenkov closed 5 days ago

ThePlenkov commented 1 week ago

Current Behavior

I would like to make sure that project is built, tested and linted before publishing. To achieve this I maintain a default target in nx.json like this

 "nx-release-publish": {
      "options": {
        "packageRoot": "dist/packages/{projectName}"
      },
      "dependsOn": [
        "build",
        "lint",
        "postbuild"
      ]
    },

However when I run a command nx release those tasks are ignored.

So I noticed that in a project graph which I get with nx graph --file=graph.json command my projects have a target nx-release-publish, but dependsOn is overwritten with dependsOn: ['^nx-release-publish'].

I quickly found a place in a code which creates this problem: in the file node_modules/nx/src/utils/package-json.js

 /**
     * Add implicit nx-release-publish target for all package.json files that are
     * not marked as `"private": true` to allow for lightweight configuration for
     * package based repos.
     */
    if (!isPrivate && !res['nx-release-publish']) {
        res['nx-release-publish'] = {
            dependsOn: ['^nx-release-publish'],
            executor: '@nx/js:release-publish',
            options: {},
        };
    }

Commenting dependsOn line in this code fully solves my problem. I'd like to propose a PR to elaborate on this apporach

Expected Behavior

If default target is maintained - it should take dependsOn from that target as it works for the rest of the project graph. I propose not to make nx-release-publish such a special target.

GitHub Repo

No response

Steps to Reproduce

  1. Add dependsOn for nx-release-publish in nx.json. For example build
  2. Try to release, project will not be built

Nx Report

Node           : 20.16.0
OS             : linux-x64
Native Target  : x86_64-linux
npm            : 10.8.1

nx (global)        : 19.6.4
nx                 : 19.6.3
@nx/js             : 19.6.3
@nx/jest           : 19.6.3
@nx/linter         : 19.6.3
@nx/eslint         : 19.6.3
@nx/workspace      : 19.6.3
@nx/devkit         : 19.6.3
@nx/eslint-plugin  : 19.6.3
@nx/plugin         : 19.6.3
@nx/rollup         : 19.6.3
@nrwl/tao          : 19.6.3
@nx/vite           : 19.6.3
@nx/web            : 19.6.3
typescript         : 5.5.4
---------------------------------------
Registered Plugins:
@nx/eslint/plugin
@nx/jest/plugin
---------------------------------------
Local workspace plugins:
         workspace-plugin

Failure Logs

No response

Package Manager Version

No response

Operating System

Additional Information

No response

ThePlenkov commented 1 week ago

22720 #21855 could be solved with this issue as well

ThePlenkov commented 1 week ago

I found also a different "hacky" way to get same, by adding a dummy section to each project package.json:

  "nx": {
    "targets": {
      "nx-release-publish": {}
    }
  }

to my opinion there must be no difference between this task and others. If we create a proper task inference plugin , there will be no need in this extra step.

JamesHenry commented 1 week ago

The fact that we run a target called nx-release-publish should be considered an implementation detail of nx release.

We can do a much better job of not having our configuration expose this implementation detail so strongly (the custom packageRoot is the only reason it needed to be exposed at all), I will work on that very soon and provide a relevant migration.

Until then, I have provided the recommended guidance for running dependent tasks before releases in other issues but I will repeat them here and add them to the documentation until the above change lands.


To me, publishing is the act of taking an already finalized artifact and adding it to a registry. It is not the point at which you prepare that relevant artifact.

This is why we have the version.preVersionCommand hook available in nx release config, to aid with performing steps other than simply updating version numbers as part of the preparation of the artifact to publish.

You do have two additional mechanisms available to you as well, however:

ThePlenkov commented 1 week ago

Unfortuanately preversion command will not work in our case because of a simple reason:

With a change introduced by this PR the only required command in CI/CD is : npx nx release publish

With properly maintained dependencies it will execute all required tasks right in CI/CD.

ThePlenkov commented 1 week ago

@JamesHenry following mentioned above problem what could be a workaround solution ( at least to stay consistend with the current approach ) is kind of prepublish command which will be called prior to publish, not version

ThePlenkov commented 1 week ago

Another idea which I really like about having release/publish as interference plugin, is that in this case we can utilze the full scope of commands, like nx run, nx run-many, nx affected. Like here I started a discussion: https://github.com/nrwl/nx/discussions/27758

It would be nice to have something like:

nx affected publish ( where publish has a dedicated executor like @nx/release-publish ) and then we can just use it as a task, not as a separate functionality. We may reach better flexibility .

JamesHenry commented 1 week ago

@ThePlenkov both of the additional options I outlined will work in your case, the npm scripts are also part of the project graph, for example. There is nothing wrong with these solutions, as you seem to be implying.

Nevertheless, I have opened a PR with a more appropriate update to the implicit target logic (commenting out a line of code which was important to functionality ensuring correctness was never going to be a viable solution). It is in draft pending discussion with the team: https://github.com/nrwl/nx/pull/27764


RE: Affected:

Affected as a mechanism would only say that a project is affected within a particular commit range.

You could use this to infer what needs to be released, but it wouldn't say what the new version should be. That's why we have different options for determining the new version: imperatively via the CLI, conventional commits, and version plans.

Conventional commits and version plans already determine what is "affected" based on the semver bump data that is encoded within them.

We could potentially explore what an affected imperative versioning would look like, but again, it's a question of what the bump should be. Some projects could be "affected" but want a patch, whereas others want a minor release.

Having only publishing be combined with affected is also problematic, because again, affected refers to a project being impacted by git changes, which is not necessarily related to the desire to publish, because the artifact you are publishing is usually mostly outside git (entirely in some cases, like the Nx repo where we version and publish from dist and use the registry as the source of truth for versioning).

Nx release needs to generically support a large number of different approaches, so we can't just add things without considering the ripple effect for all use-cases.

ThePlenkov commented 1 week ago

I really like this overview BTW. It shows all tasks which were performed on publish. Exactly what I wanted image