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

Local Task Caching for all Revelant tasks #5134

Merged
merged 15 commits into from
Mar 17, 2023

Conversation

ZackDeRose
Copy link
Contributor

@ZackDeRose ZackDeRose commented Mar 15, 2023

What this does

  • Makes all relevant tasks cacheable
  • From the perspective of calling the existing pnpm scripts, all scripts should result in the same behavior as before; with the exceptions that:
    1. Nx is now being used as the task runner
    2. Some commands that were sequential are now parallelized where it made sense to parallelize
    3. Some commands are cacheable where it made sense to cache task results

@vercel
Copy link

vercel bot commented Mar 15, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
query ⬜️ Ignored (Inspect) Mar 16, 2023 at 7:54PM (UTC)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 15, 2023

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:

Sandbox Source
@tanstack/query-example-react-basic-typescript Configuration
@tanstack/query-example-solid-basic-typescript Configuration
@tanstack/query-example-svelte-basic Configuration
@tanstack/query-example-vue-basic Configuration

@codecov-commenter
Copy link

codecov-commenter commented Mar 15, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.31 🎉

Comparison is base (b8b0562) 91.90% compared to head (f5390fd) 92.22%.

📣 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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ZackDeRose ZackDeRose force-pushed the test-process-caching branch from ec895b9 to 4dc5a6d Compare March 15, 2023 06:50
@ZackDeRose ZackDeRose changed the title Local Task Caching added for Eslint for packages Local Task Caching for all Revelant tasks Mar 16, 2023
Comment on lines +19 to +23
"pluginsConfig": {
"@nrwl/js": {
"analyzeSourceFiles": false
}
},
Copy link
Contributor Author

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!

nx.json Outdated
Comment on lines 58 to 70
"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"]
},
Copy link
Contributor Author

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!

Copy link
Contributor Author

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!

package.json Outdated
Comment on lines 26 to 32
"nx": {
"includedScripts": [
"test:format",
"build",
"test:build"
]
},
Copy link
Contributor Author

@ZackDeRose ZackDeRose Mar 16, 2023

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:eslint
  • test:types
  • build
  • build:types

scripts, so that calling these do not infinitely recurse when we call pnpm build for example!

@@ -0,0 +1,28 @@
{
Copy link
Contributor Author

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 @@
{
Copy link
Contributor Author

@ZackDeRose ZackDeRose Mar 16, 2023

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:build": "nx test:build root",
"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\"",
Copy link
Contributor Author

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": ["**/*"]
Copy link
Contributor Author

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.

project.json Outdated
Comment on lines 7 to 18
"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
}
},
Copy link
Contributor Author

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!

@TkDodo TkDodo merged commit 8e13afa into TanStack:main Mar 17, 2023
@TkDodo
Copy link
Collaborator

TkDodo commented Mar 17, 2023

I think I spotted an issue with the test:format task. I've formatted everything correctly, run the task. Then made a change in a tsx file, run the task again, but it just reads from the cache:

> nx test:format root


> nx run root:test:format  [existing outputs match the cache, left as is]

I then tried with --skip-nx-cache and the error showed up:

> nx test:format root "--skip-nx-cache"


> nx run root:"test:format"

> query@ prettier /Users/dodo/work/TanStackQuery
> prettier --plugin-search-dir . "{packages,examples}/**/src/**/*.{md,js,jsx,ts,tsx,json,vue,svelte}" "--check"
Checking formatting...
[warn] packages/react-query-devtools/src/devtools.tsx
[warn] Code style issues found in the above file. Forgot to run Prettier?
 ELIFECYCLE  Command failed with exit code 1.
 >  NX   ERROR: Something went wrong in run-commands - Command failed: pnpm run prettier --check

@ZackDeRose
Copy link
Contributor Author

I must've misconfigured the cache here - on it now!

@ZackDeRose
Copy link
Contributor Author

fix up: #5147

^ demo video to show this working now :)

We should definitely be vigilant against false positives like this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants