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

fix(core): add project.json to nx implicit dependencies #8825

Closed
wants to merge 3 commits into from

Conversation

meeroslav
Copy link
Contributor

@meeroslav meeroslav commented Feb 3, 2022

Current Behavior

The project.json configuration (including tags) is not set as implicitDependencies by default so any changes to those files will not influence the hashing and might use an invalid cached version. This is evident with linting during the tags change. The lint runner does not recognize the change and uses the wrong cached output.

Expected Behavior

The project.json file should be part of implicitDependencies so the changes would influence caching.

Related Issue(s)

Fixes #8427

@nx-cloud
Copy link

nx-cloud bot commented Feb 3, 2022

@vercel
Copy link

vercel bot commented Feb 3, 2022

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/6PvQaFkiXMuX4Dd62XQ8RkMh6TDG
✅ Preview: Canceled

[Deployment for 3e0856f canceled]

@meeroslav meeroslav self-assigned this Feb 4, 2022
@meeroslav meeroslav marked this pull request as ready for review February 4, 2022 11:23
@AgentEnder
Copy link
Member

Hmmmmm, this means that changing any project's tags will cause the whole graph to register as affected for CI runs right? I wouldn't think that would be desired, but cannot think of a good way to invalidate the linting without everything else.

@meeroslav
Copy link
Contributor Author

@AgentEnder, this issue has two subproblems:

  • How to make sure changes in tags trigger the affected graph (currently they don't)
  • How to make sure changes in tags invalidate the cached artifacts for the current project and all its ancestors.

Is see three approaches to the problem:

  • (Current fix) Setting tags to implicitDependencies. This ensures both of the problems are solved but at an expense of having an overarching footprint (and impacting thus performance). It's the same functionality that existed while tags section lived in nx.json and later in workspace/angular.json. It's the quickest but the least elegant solution.
  • Extend ImplicitDependencyEntry to accept a list of targets being affected by the change. Currently, the value accepts * for all or string[] for the list of affected projects. This would still run the full lint if any tag changes, which is not ideal, but at least other targets would not be affected. It's a bit more complicated solution and required additional DSL syntax to distinguish targets from project names.
  • Make separate tags-related affected graph calculation that is only initiated if the target is linter. The changes would have to be made in the affected graph calculation (to make sure all affected nodes are detected) and in the hasher (to ensure we don't read stale info from the cache). It's the large-scale change that would produce a surgical impact on the graph. It might be tricky in the cases where we run multiple targets since the graph could no longer be shared.

@meeroslav meeroslav marked this pull request as draft February 21, 2022 09:58
@AgentEnder
Copy link
Member

@meeroslav I kinda like the 2nd solution there, but almost would rather it be which executors are affected rather than which targets. I.e. if a workspace was setup with something like this:

{
  "targets": {
    "eslint": {...},
    "prettier": {...},
    "lint": {
        // ... some stuff
       dependsOn: [
         {
            "target": "eslint"
            "project": "self"
         },
         {
            "target": "prettier"
            "project": "self"
         },
       ]
    }
  }
}

then in nx.json, you'd probably want the implicit dependency to specifically hit that eslint target. This could be done by just specify eslint as the target, but a consumer could name that any arbitrary thing. If we were to use the executor as the key, we could key off of @nrwl/linter:eslint

@AgentEnder
Copy link
Member

If we want to just land the fix quickly, than the first solution may be fine for a quick fix, but should look into others for long term IMHO.

@meeroslav
Copy link
Contributor Author

meeroslav commented Feb 23, 2022

Solution 2 might look something like this:

  // ...other nx.json config
  "implicitDependencies": {
    "package.json": {
      "dependencies": "*",
      "devDependencies": "*"
    },
    ".eslintrc.json": "executor:@nrwl/linter*", // to be a bit more flexible with naming executors
    "apps/**/project.json": {
      "tags": ["executor:@nrwl/linter*", "executor:my-other-executor"]
    },
    "libs/**/project.json": {
      "tags": "executor:@nrwl/linter*"
    }
  },
  // ...other nx.json config

@meeroslav
Copy link
Contributor Author

Closing this in favour of #9110

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eslint-plugin-nx ignores implicitDependencies
2 participants