nrwl / nx

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

Yarn+NX rebuilds all workspaces with any package.json dependency change #10842

Closed gbisaga closed 1 year ago

gbisaga commented 2 years ago

Current Behavior

NX rebuilds all yarn workspaces when the package.json dependencies of any workspace change. Since we are using internally-published packages and iterating them frequently, this causes the project to rebuild way too often.

Expected Behavior

NX only rebuilds workspaces with actual dependency changes.

Steps to Reproduce

  1. Clone https://github.com/gbisaga/nx-rebuild-all
  2. yarn install --frozen-lockfile
  3. yarn build:all
  4. yarn workspace @ps-refarch/nxpackage3 add jsonwebtoken
  5. yarn build:all

Expected behavior: only @foo-monorepo/nxpackage3 rebuilds Actual behavior: all 3 workspaces rebuild

Failure Logs

Environment

Node : 16.13.1 OS : win32 x64 yarn : 1.22.18

nx : 14.3.1 @nrwl/angular : Not Found @nrwl/cypress : Not Found @nrwl/detox : Not Found @nrwl/devkit : 14.3.1 @nrwl/eslint-plugin-nx : Not Found @nrwl/express : Not Found @nrwl/jest : 14.3.1 @nrwl/js : Not Found @nrwl/linter : 14.3.1 @nrwl/nest : Not Found @nrwl/next : Not Found @nrwl/node : Not Found @nrwl/nx-cloud : Not Found @nrwl/nx-plugin : Not Found @nrwl/react : Not Found @nrwl/react-native : Not Found @nrwl/schematics : Not Found @nrwl/storybook : Not Found @nrwl/web : Not Found @nrwl/workspace : 14.3.1 typescript : 4.7.3

Community plugins:

gregjacobs commented 2 years ago

+1, and I just noticed the same while evaluating Nx. We update dependencies quite often in our current Rush repository, and I could foresee this creating a lot of full repo rebuilds in Nx.

Instead of hashing the dependencies/devDependencies text inside package.json, it seems Nx needs a mechanism to hash based on the particular npm dependencies that are actually used by the package + its build tools. Perhaps the root-level package-lock.json or yarn.lock could be used for this purpose? (Store a hash of the dependency graph created for any given package X?) Or is the better solution to have a separate package.json file for each package?

gbisaga commented 2 years ago

Greg, we do have a separate package.json for each of the components, which is why the current behavior seems unexpected. I am guessing the reason it's rebuilding is because yarn "hoists" the package installs up to the base directory - i.e. even though I installed this package in the @foo-monorepo/nxpackage3 component, it actually installs it in the root node_modules. Technically, this means other components could access it there, although that would be incorrect. I feel like it should be hashing the separate component dependencies instead of all of them combined as it seems to be doing.

dan-cooke commented 1 year ago

This is because of implicitDependencies

Quite a few discussions and issues around this in NX

https://github.com/nrwl/nx/discussions/5580 https://github.com/nrwl/nx/discussions/5580

This is the main issue tracking this https://github.com/nrwl/nx/issues/1972 and its been sitting for 2 years now.

Its a major problem with NX - if you have a large monorepo, any changes to package.json will trigger nx affected for all projects and libraries - even though only one project may use the library.

This really wrecks havock on CI builds when you are building 10+ apps in parallel in docker - no one change should ever require that many projects to rebuilt.

ianldgs commented 1 year ago

Yeah, simply adding a new dependency to one library, even with pnpm workspaces, is causing all packages to be affected. I've tried with nx@15.8.6, which is supposed to have all the new lockfile parsing, but no luck. @meeroslav is there any new change that could fix this? It's the whole point of NX to not affect everything all the time.

meeroslav commented 1 year ago

@ianldgs, lock file parsing affects the hashing and caching, but not the affected graph. The affected graph is agnostic of the target and therefore can not know if your target might depend on that changed package.

The entire graph will be affected by any change to the lock file, but then the caching should return cache hits if the given package does not affect a project with a target run. I will have to investigate why the OP didn't get a cache hit in this case.

Unfortunately, despite being clear from the command that this update could not influence other projects, we only analyse the lock file and don't know how this change was made.

ianldgs commented 1 year ago

Understood, @meeroslav! Thanks for looking.

So, in my case it's clear why the packages didn't get a cache hit: there's a versioning step that bumps the affected packages, therefore invalidating the cache. Maybe we could only run this in a "release" build, once a day, but I really like merging to master and having the npm package I just created/fixed be published to the registry.

At some point in NX < 14 (I think) (before named inputs), any change to the root package.json would affect all packages. That was annoying, but after integrating npm/yarn/pnpm workspaces we could declare dependencies per package, so the change in the local package.json would only affect their respective packages. Lockfile used to be ignored in that case IIRC. IMO that was a better flow, as some of our packages require explicit declaration anyway. And if a dependency at the root was changed, it was much more likely that it really affected all packages (e.g. upgrading nx).

Edit: went crazy and made a proposal PR 😝

meeroslav commented 1 year ago

Unfortunately, @gbisaga the hashing would only work in your case if you would use one of the @nrwl/* executors, as these are the only ones for which we can guarantee that change in lock file did not influence your build target.

The hashing does work per project basis but we also include the target in the hash (in your case build that is triggering tsc).

When running arbitrary commands as build (e.g. tsc or any node script), we cannot tell when that command might depend on and whether the change in lock-file could have influenced it.

We had some discussions of adding additional flags to the target configuration where you would be able to manually override that, but currently, this block of code is working against you as it grabs entire externalNodes (e.g., parsed lock-file):

https://github.com/nrwl/nx/blob/master/packages/nx/src/hasher/hasher.ts#L372-L391

You can solve this issue by using @nrwl/js package and @nrwl/js:tsc executor.

github-actions[bot] commented 1 year ago

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.