Jaybell/remove-irrelevant-paths-from-tsconfig-in-hash #5587

yharaskrik · 2021-05-07T02:36:32Z

I realize the hasher is super core to Nx and @vsavkin has mentioned before that it is not liberally changed but the linked issue is one of the things stopping us from using incremental building right now as we refactor a big legacy codebase with hundreds of libraries that will be merged which means we are constantly breaking the cache use to tsconfig.json/nx.json being updated. I hope this Pr is helpful in solving that issue.

ISSUES CLOSED: #5283

Current Behavior

Currently as discussed in issues closed, when affecting tsconfig/nx.json in ways that will not affect the outcome of targets those targets caches are broken. Example: When generating a library, the caches for all builds are broken which means everything needs to be rebuilt again.

Expected Behavior

When making changes to tsconfig and/or nx.json that should not affect the outcome of targets then those changes should not be considered for the caches hash.

This PR does two things:

Removes the global nx.json hash, reasoning: the nx.json already has it's implicitDeps being hashed and the specific project in the nx.json projects object is also being hashed. This means we should not have to also hash the entire file.
Hashes the entire tsconfig file except the compilerOptions.paths of projects not currently being hashed, ie. only the current projects path configuration is now hashed, all others are discarded. This will prevent the generation of a new library breaking the cache for nx targets. Note: I added some typings for TsConfigJsonConfiguration because there was none and couldn't find any. I didn't add every property available in the file but can if that is something that is wanted.

Related Issue(s)

Fixes #5283

vercel · 2021-05-07T02:36:36Z

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/FZhgCeWvUUamuKqDbEC5iQQ7N6b3
✅ Preview: https://nx-dev-git-fork-yharaskrik-jaybell-remove-entire-ts-7e04be-nrwl.vercel.app

nx-cloud · 2021-05-07T02:44:29Z

Nx Cloud Report

CI ran the following commands for commit b8e755b. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status	Command
	`nx affected --target=build --base=c17fb12ee6b209c196231512b36d0e6f5bb83d3d --parallel --max-parallel=3`
	`nx affected --target=e2e --base=c17fb12ee6b209c196231512b36d0e6f5bb83d3d`
	`nx affected --target=lint --base=c17fb12ee6b209c196231512b36d0e6f5bb83d3d --parallel --max-parallel=4`
	`nx affected --target=test --base=c17fb12ee6b209c196231512b36d0e6f5bb83d3d --parallel --max-parallel=2`
	`nx build typedoc-theme`

Sent with 💌 from NxCloud.

nx-cloud · 2021-05-07T02:44:29Z

Nx Cloud Report

CI ran the following commands for commit afd4833. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status	Command
	`nx affected --target=test`
	`nx run-many --target=test --projects=workspace`
	`nx run-many --target=test --projects=angular`
	`nx run-many --target=test --projects=cli`
	`nx run-many --target=test --projects=cypress`
	`nx run-many --target=test --projects=dep-graph-dep-graph`
	`nx run-many --target=test --projects=devkit`
	`nx run-many --target=test --projects=eslint-plugin-nx`
	`nx run-many --target=test --projects=express`
	`nx run-many --target=test --projects=gatsby`
	`nx run-many --target=test --projects=jest`
	`nx run-many --target=test --projects=linter`
	`nx run-many --target=test --projects=nest`
	`nx run-many --target=test --projects=next`
	`nx run-many --target=test --projects=node`
	`nx run-many --target=test --projects=nx-dev-ui-common`
	`nx run-many --target=test --projects=nx-plugin`
	`nx run-many --target=test --projects=react`
	`nx run-many --target=test --projects=storybook`
	`nx run-many --target=test --projects=web`

Sent with 💌 from NxCloud.

manfredsteyer · 2021-05-14T19:17:20Z

I love the idea!

vsavkin · 2021-05-27T17:57:34Z

Thank you for the PR!

We have added an ability to provide custom hasher for executors (e.g., packages/linter/src/executors/eslint/hasher.ts). Different use case but somewhat relevant.
As you probalby have noticed, we no longer hash the whole nx.json, but only the relevant bits. So the only thing in question is tsconfig.base.json.
At this point, only adding a new lib will invalidate the cache (cause of tsconfig), updating the lib won't invalidate the cache for the whole workspace. Your tsconfig change will fix it but it assumes that project name is used in tsconfig, which might not be the case. You can use any mappings you want in your tsconfig, they don't have to match the project name. So it can be a breaking change for "unusual" workspaces.

We can add a tasks runner option (e.g., selectively cache tsconfig) which is false by default but can be set to true in nx.json, and then we can do what you do with tsconfig.json. The hasher already takes options, so you can just use this option. I think for a lot of workspaces, the behavior in this PR is good, but for some it's broken.

Does it make sense?

yharaskrik · 2021-05-27T18:00:57Z

Oh I did not know #1 was a thing! I am going to have to take a look at that.

#2 was what prompted this PR since I figured the same logic could be applied to tsconfig.base.json

I think having the option to enable/disable this PR in nx.json would be awesome, Would you like me to add that to this PR and then write some tests for it?

Thanks for the feedback Victor!

yharaskrik · 2021-05-27T22:46:33Z

Hey Victor, I went in and added that option for caching the tsconfig and updated the tests to test for that case. I had to expand the test cases a bit to be a bit more "realistic" so that I could calculate the ts alias and such so the tests are expecting more realistic hashes now for projects. Not sure where I should document that flag though. Let me know if you would like me to add it to the docs somewhere.

vsavkin

Sorry for the late reply. I think overall it looks good. Left some minor feedback.

vsavkin · 2021-07-02T17:49:42Z

packages/workspace/src/core/hasher/hasher.ts

+      const fileHashes = fileNames.map((file) => {
+        const hash = this.fileHasher.hashFile(file);
+        return { file, hash };
+      });


why was hashGlobalConfig removed?

Hmm that is a good question, I did this a while ago. I don't see a reason it should be so I will just add it back in.

Oh actually, it was removed because of #1 the original PR description.

Removes the global nx.json hash, reasoning: the nx.json already has it's implicitDeps being hashed and the specific project in the nx.json projects object is also being hashed. This means we should not have to also hash the entire file.

Is that not true? If so I will leave the global hash in (as the PR stands)

packages/workspace/src/core/hasher/hasher.ts

packages/tao/src/shared/tsconfig.ts

vsavkin · 2021-07-02T18:03:11Z

Could you also cover the option in configuration.md (we have three version of this file)?

Adds the ability to pass in number of nodes and current node to affected run commands so that target runs can be split evenly across nodes ISSUES CLOSED: nrwl#5283

…x hash commit files correctly

…hashing

…ld method

yharaskrik · 2021-07-05T02:18:53Z

@vsavkin to confirm the 3 files you are referring to are angular/../configuration.md, react/../configuration.md and node/.../configuration.md?

yharaskrik · 2021-08-02T20:53:00Z

I believe this should be good to be reviewed again. Requested changes have been made.

vsavkin · 2021-08-05T19:24:42Z

Thank you! I made some minor changes here: #5587

I'm going to merge that PR, so I'm closing this one.

yharaskrik · 2021-08-05T19:26:13Z

@vsavkin sounds good! I think you linked the wrong PR though haha.

yharaskrik · 2021-08-05T19:29:20Z

For anyone else looking for the PR victor is referring to: #6623

github-actions · 2023-03-20T11:05:37Z

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.

vercel bot deployed to Preview May 7, 2021 02:36 View deployment

vsavkin self-assigned this May 18, 2021

vercel bot deployed to Preview May 27, 2021 22:43 View deployment

yharaskrik force-pushed the jaybell/remove-entire-tsconfig-from-hasher branch from b56bfef to 8dd8b43 Compare May 27, 2021 22:45

vercel bot deployed to Preview May 27, 2021 22:49 View deployment

vercel bot deployed to Preview May 27, 2021 23:02 View deployment

yharaskrik mentioned this pull request Jun 1, 2021

Running affected:libs rebuilds everything when new publishable lib is added #5573

Closed

vsavkin requested changes Jul 2, 2021

View reviewed changes

yharaskrik added 6 commits July 4, 2021 19:09

fix(core): intelligently hash tsconfig and remove global nx hash

eb9baab

Adds the ability to pass in number of nodes and current node to affected run commands so that target runs can be split evenly across nodes ISSUES CLOSED: nrwl#5283

fix(core): fix(core): intelligently hash tsconfig and remove global n…

3b00328

…x hash commit files correctly

fix(core): fix(core): add option in nx json to cache tsconfig during …

41562a4

…hashing

fix(core): fix(core): rebase onto master and fix incorrect usage of o…

5ae9c99

…ld method

fix(core): update ts config cache flag name

861f707

fix(core): fix(core): update ts config cache flag name

f68e371

yharaskrik force-pushed the jaybell/remove-entire-tsconfig-from-hasher branch from 7bd252d to f68e371 Compare July 5, 2021 02:16

vercel bot temporarily deployed to Preview July 5, 2021 02:16 Inactive

yharaskrik requested a review from vsavkin July 5, 2021 02:16

docs(core): add explanation of new flag for caching ts config

f060d4e

vercel bot deployed to Preview July 5, 2021 02:25 View deployment

docs(core): docs(core): add default to description

b8e755b

vercel bot deployed to Preview July 5, 2021 02:36 View deployment

vsavkin closed this Aug 5, 2021

github-actions bot locked as resolved and limited conversation to collaborators Mar 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jaybell/remove-irrelevant-paths-from-tsconfig-in-hash #5587

Jaybell/remove-irrelevant-paths-from-tsconfig-in-hash #5587

yharaskrik commented May 7, 2021

vercel bot commented May 7, 2021 •

edited

Loading

nx-cloud bot commented May 7, 2021 •

edited

Loading

nx-cloud bot commented May 7, 2021

manfredsteyer commented May 14, 2021

vsavkin commented May 27, 2021

yharaskrik commented May 27, 2021

yharaskrik commented May 27, 2021

vsavkin left a comment

vsavkin Jul 2, 2021

yharaskrik Jul 5, 2021

yharaskrik Jul 5, 2021

vsavkin commented Jul 2, 2021

yharaskrik commented Jul 5, 2021

yharaskrik commented Aug 2, 2021

vsavkin commented Aug 5, 2021

yharaskrik commented Aug 5, 2021

yharaskrik commented Aug 5, 2021

github-actions bot commented Mar 20, 2023

Jaybell/remove-irrelevant-paths-from-tsconfig-in-hash #5587

Jaybell/remove-irrelevant-paths-from-tsconfig-in-hash #5587

Conversation

yharaskrik commented May 7, 2021

Current Behavior

Expected Behavior

Related Issue(s)

vercel bot commented May 7, 2021 • edited Loading

nx-cloud bot commented May 7, 2021 • edited Loading

Nx Cloud Report

nx-cloud bot commented May 7, 2021

Nx Cloud Report

manfredsteyer commented May 14, 2021

vsavkin commented May 27, 2021

yharaskrik commented May 27, 2021

yharaskrik commented May 27, 2021

vsavkin left a comment

Choose a reason for hiding this comment

vsavkin Jul 2, 2021

Choose a reason for hiding this comment

yharaskrik Jul 5, 2021

Choose a reason for hiding this comment

yharaskrik Jul 5, 2021

Choose a reason for hiding this comment

vsavkin commented Jul 2, 2021

yharaskrik commented Jul 5, 2021

yharaskrik commented Aug 2, 2021

vsavkin commented Aug 5, 2021

yharaskrik commented Aug 5, 2021

yharaskrik commented Aug 5, 2021

github-actions bot commented Mar 20, 2023

vercel bot commented May 7, 2021 •

edited

Loading

nx-cloud bot commented May 7, 2021 •

edited

Loading