nx: Upgrading NX 12 to 13 broke "nx run many" with "--only-failed"

Current Behavior

After upgrading from NX 12 to 13 the command nx run-many --all --target=lint --only-failed will result in this error:

TypeError: Cannot read properties of null (reading 'getResult')
    at /Users/svzi/development/xyz/apps/node_modules/@nrwl/workspace/src/command-line/run-many.js:47:105
    at Array.filter (<anonymous>)
    at applyOnlyFailed (/Users/svzi/development/xyz/apps/node_modules/@nrwl/workspace/src/command-line/run-many.js:47:47)
    at Object.<anonymous> (/Users/svzi/development/xyz/apps/node_modules/@nrwl/workspace/src/command-line/run-many.js:22:34)
    at Generator.next (<anonymous>)
    at fulfilled (/Users/svzi/development/xyz/apps/node_modules/tslib/tslib.js:114:62)

This is the corresponding function:

function applyOnlyFailed(projectsNotExcluded, nxArgs, env) {
    return Object.values(projectsNotExcluded).filter((n) => !nxArgs.onlyFailed || !env.workspaceResults.getResult(n.name));
}

For whatever reason env.workspaceResults is null (we’ve debugged it).

Expected Behavior

It should run as expected. When we remove the --only-failed parameter everything works as expected. But we have multiple apps and several libs in our project, so we really like that parameter.

Steps to Reproduce

We upgraded an existing NX 12 workspace with Angular 12 to NX 13 / Angular 13. That’s all. We run the migration commands as we’re supposed to do. Everything else is looking good so far, just the issue mentioned above.

Environment

  Node : 16.11.1
  OS   : darwin arm64
  npm  : 8.0.0

  nx : 13.3.0-beta.2
  @nrwl/angular : 13.3.0-beta.2
  @nrwl/cli : 13.3.0-beta.2
  @nrwl/cypress : 13.3.0-beta.2
  @nrwl/devkit : 13.1.3
  @nrwl/eslint-plugin-nx : 13.3.0-beta.2
  @nrwl/express : undefined
  @nrwl/jest : 13.3.0-beta.2
  @nrwl/linter : 13.3.0-beta.2
  @nrwl/nest : undefined
  @nrwl/next : undefined
  @nrwl/node : 13.3.0-beta.2
  @nrwl/nx-cloud : undefined
  @nrwl/react : undefined
  @nrwl/react-native : undefined
  @nrwl/schematics : undefined
  @nrwl/tao : 13.3.0-beta.2
  @nrwl/web : undefined
  @nrwl/workspace : 13.3.0-beta.2
  @nrwl/storybook : 13.3.0-beta.2
  @nrwl/gatsby : undefined
  typescript : 4.4.4
  rxjs : 6.6.0
  ---------------------------------------
  Community plugins:
  	 @angular/animations: 13.0.2
  	 @angular/common: 13.0.2
  	 @angular/compiler: 13.0.2
  	 @angular/core: 13.0.2
  	 @angular/forms: 13.0.2
  	 @angular/google-maps: 13.0.2
  	 @angular/platform-browser: 13.0.2
  	 @angular/platform-browser-dynamic: 13.0.2
  	 @angular/router: 13.0.2
  	 @angular/service-worker: 13.0.2
  	 @ionic/angular: 5.9.1
  	 @angular-devkit/architect: 0.1300.3
  	 @angular-devkit/build-angular: 13.0.3
  	 @angular-eslint/builder: 13.0.1
  	 @angular/compiler-cli: 13.0.2
  	 @angular/language-service: 13.0.2
  	 @angular/localize: 13.0.2
  	 @storybook/angular: 6.4.0-rc.3

Any help would be really much appreciated!

Best, Sven

About this issue

Original URL
State: closed
Created 3 years ago
Reactions: 9
Comments: 25 (5 by maintainers)

Most upvoted comments

Really sorry for the late reply folks. This is all the context:

–only-failed has caused a lot of issues because it was the only stateful part about Nx (things that we had to mutate in place between runs), so it caused bugs and special cases. But we didn’t remove it cause it was hard but because it no longer made sense.

We introduced it early on before computation caching was implemented, task dependencies were implemented etc. When we introduced --only-failed most of the task graphs Nx worked with were flat (e.g., every task was independent). A lot of projects right now aren’t flat: tasks can depend on other tasks.

When we examined the current situation a few months ago, we saw that --only-failed doesn’t solve the problem holistically. If you are running, say nx build myapp, and that app depends on 1000 buildable libs, it’s the caching that enables the min amount of recompute. --only-failed would still rerun myapp (same stuff about e2e tests depending on builds etc.)

--only-failed in this case doesn’t make much sense. What failed isn’t the app, but something that the app depends on. If you run nx build myapp again, it will not rebuild the libs that were built successfully the first time. It will only rerun the ones that failed.

In other words, running the same command second time only reruns what failed (or skipped) the first time–the rest is retrieved from cache (or left as is if the right files are already in the right place). So in practice any command right now sort of runs with --only-failed.

The problem we had is that you would see too much noise in the terminal output–everything cached will still be printed out (at the top). The tasks that are being rerun would be printed last. We knew it’s not ideal, and at the time we were already working on the new terminal output strategy we launched a week ago. With the new terminal output strategy, rerunning something twice doesn’t generate any noise in the terminal output.

It took us a bit longer to land the new terminal output strategy that we hoped for, and we had to remove --only-failed before that (cause of how we manage the life cycle). I apologize for that. I know it wasn’t ideal.

Also, I added a warning saying that --only-failed is going to be skipped, but I made a mistake, and the exception was thrown before we could print the warning. I apologize for this as well.

Summary:

With caching and the new terminal output, rerunning any command essentially has --only-failed, and it works for non-flat graphs. I know it’s not apple-to-apples and that it is possible to come up with some scenario where the previous DX was better. But the prev behavior only worked for flat graphs and was costly to maintain. You could always copy the project names and run them separately as a workaround, but we rarely would have to do that cause caching will do its job.
Jason is going to submit a PR to surface the warning properly.

Hopefully it makes sense. Thank you!

vsavkin on Jan 28, 2022

@vsavkin @FrozenPandaz I can now confirm that retry by just executing the command (ie test) and relying on the cache to skip the successful ones and only run the failed ones in CI causes many 1000s of lines of repeated logs!

The smart terminal logging is not that smart when you apply it to file logging.

Any ideas on how to avoid these repeated logs for the successful libs and only show the logs for failed and actually retried ones?

gkamperis on Feb 18, 2022

@vsavkin @FrozenPandaz @AgentEnder it has been 2 months since the issue was opened…

would be good to get some insight on what the plan is, why the change was made and what the proposed workaround would be if it is intended to be removed

pls help!

gkamperis on Jan 26, 2022

hello @AgentEnder @vsavkin any info on this? it is preventing our move to ng 13

gkamperis on Jan 4, 2022