-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Local Task Caching for all Revelant tasks #5134
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 2e61af4:
|
Codecov ReportPatch coverage has no change and project coverage change:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #5134 +/- ##
==========================================
+ Coverage 91.90% 92.22% +0.31%
==========================================
Files 111 111
Lines 4188 4229 +41
Branches 1083 1099 +16
==========================================
+ Hits 3849 3900 +51
+ Misses 318 308 -10
Partials 21 21 see 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
ec895b9 to
4dc5a6d
Compare
| "pluginsConfig": { | ||
| "@nrwl/js": { | ||
| "analyzeSourceFiles": false | ||
| } | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
short explanation: this exlicitely states our depency graph strategy - which is to analyze deps based on package.json files. This was the behavior we are already using.
long explanation: I added the @nrwl/workspace briefly to this PR, and while I ended up not needing it, it appears that this changes the default behavior to determine deps by source file analysis. I want to leave this here so this isn't a surprise in case we end up introducing other nx plugins later on!
| "test:types": { | ||
| "outputs": [ | ||
| "{projectRoot}/build/lib/**/*.d.ts", | ||
| "{projectRoot}/build/.tsbuildinfo" | ||
| ], | ||
| "inputs": ["default", "^public"], | ||
| "dependsOn": ["^test:types"] | ||
| }, | ||
| "build:types": { | ||
| "outputs": ["{projectRoot}/build"], | ||
| "inputs": ["default", "^public"], | ||
| "dependsOn": ["^build:types"] | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, test:types and build:types scripts tend to do the same things:
test:types: tsc
build:types: tsc --build
If I'm not mistaken, the build option is true by default, so these are equivalent. If we want to not write to disk on the test:types I think we can use: test:types --build=false, but we'll need to make sure that the builds we depend on happen first for this to work correctly!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TkDodo - if we want to make the adjustment above, can you create a new issue for this? And feel free to assign to me.
I'd like for this PR to be a like-for-like as much as possible so we don't change any behaviors!
| "nx": { | ||
| "includedScripts": [ | ||
| "test:format", | ||
| "build", | ||
| "test:build" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this property creates an "include list" of the package.json scripts that nx will include in it's task graph.
If this property is not set, all package scripts become "tasks" that nx can find and run (for example via nx test:types @tanstack/query-core and nx run-many --target=test:types)
My intention in adding this property is actually to make sure we exclude:
test:eslinttest:typesbuildbuild:types
scripts, so that calling these do not infinitely recurse when we call pnpm build for example!
| @@ -0,0 +1,28 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file creates the concept of a "project" for nx at the source root, so we can define and run some root-level scripts
| @@ -0,0 +1,8 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this defines a scripts project in our workspace for nx inside the scripts directory.
With this, we can now call: nx test:eslint scripts to lint our scripts, and what's cooler, we can now just "add this to the pile" by calling nx run-many --target=test:eslint inside of our package scripts/ci stuff :)
| "test:types": "nx run-many --target=test:types --parallel=5", | ||
| "build": "nx run-many --target=build --projects=root,@tanstack/eslint-plugin-query", | ||
| "build:types": "nx run-many --target=build:types --parallel=5", | ||
| "watch": "concurrently --kill-others \"rollup --config rollup.config.js -w\" \"pnpm run build:types --watch\"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might be able to use nx watch here for the watch command - but I'll keep that separate from this pr
| } | ||
| } | ||
| }, | ||
| "implicitDependencies": ["**/*"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This marks our root nx project as dependent on all packages. We use this to inform our caching - as you can see in the nx.json file, the inputs for the build target is the public files of all dependencies.
| "build": { | ||
| "executor": "nx:run-commands", | ||
| "options": { | ||
| "commands": [ | ||
| "rollup --config rollup.config.js", | ||
| "nx run-many --target=build:types", | ||
| "nx build @tanstack/eslint-plugin-query", | ||
| "nx build @tanstack/svelte-query" | ||
| ], | ||
| "parallel": true | ||
| } | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build should do these 4 things, and it can actually do all 4 in independently if we want, so that's what we're doing here.
Interestingly @tanstack/svelte-query has it's own build step, but all other packages are built in 1 go with the rollup command it appears.
@TkDodo - let me know if there's a preference here, we might be able to get some boost by parallelizing the package builds and running them individually (this also allows the cache to be more granular - as it stands we need to "invalidate" the build cache for all projects whenever a single project's public code is touched 🥶
If we do want to break it up, let's do it in another PR! As I mention elsewhere, I'd like to keep this one as "like-for-like" as possible!
|
I think I spotted an issue with the I then tried with |
|
I must've misconfigured the cache here - on it now! |
|
fix up: #5147 ^ demo video to show this working now :) We should definitely be vigilant against false positives like this! |
What this does