Skip to content

fix(paths): use customConditions to define internal imports #8935

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

Merged
merged 13 commits into from
Apr 4, 2025

Conversation

tylersayshi
Copy link
Contributor

@tylersayshi tylersayshi commented Apr 2, 2025

I found a technique in the arktype monorepo that uses customConditions to define imports to be used in the monorepo. It seems like a really clean solution for this to me, though it does mean updating each package.json

a good description of the different approaches at this problem and why customConditions is ultimately a good solution for it: https://colinhacks.com/essays/live-types-typescript-monorepo

fixes #8865

Copy link

nx-cloud bot commented Apr 2, 2025

View your CI Pipeline Execution ↗ for commit c3b2554.

Command Status Duration Result
nx affected --targets=test:sherif,test:knip,tes... ✅ Succeeded 4m 44s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 1m 46s View ↗

☁️ Nx Cloud last updated this comment at 2025-04-04 19:21:14 UTC

Copy link

pkg-pr-new bot commented Apr 2, 2025

More templates

@tanstack/angular-query-devtools-experimental

npm i https://pkg.pr.new/@tanstack/angular-query-devtools-experimental@8935

@tanstack/angular-query-experimental

npm i https://pkg.pr.new/@tanstack/angular-query-experimental@8935

@tanstack/eslint-plugin-query

npm i https://pkg.pr.new/@tanstack/eslint-plugin-query@8935

@tanstack/query-async-storage-persister

npm i https://pkg.pr.new/@tanstack/query-async-storage-persister@8935

@tanstack/query-broadcast-client-experimental

npm i https://pkg.pr.new/@tanstack/query-broadcast-client-experimental@8935

@tanstack/query-core

npm i https://pkg.pr.new/@tanstack/query-core@8935

@tanstack/query-devtools

npm i https://pkg.pr.new/@tanstack/query-devtools@8935

@tanstack/query-sync-storage-persister

npm i https://pkg.pr.new/@tanstack/query-sync-storage-persister@8935

@tanstack/react-query

npm i https://pkg.pr.new/@tanstack/react-query@8935

@tanstack/query-persist-client-core

npm i https://pkg.pr.new/@tanstack/query-persist-client-core@8935

@tanstack/react-query-devtools

npm i https://pkg.pr.new/@tanstack/react-query-devtools@8935

@tanstack/react-query-next-experimental

npm i https://pkg.pr.new/@tanstack/react-query-next-experimental@8935

@tanstack/react-query-persist-client

npm i https://pkg.pr.new/@tanstack/react-query-persist-client@8935

@tanstack/solid-query-devtools

npm i https://pkg.pr.new/@tanstack/solid-query-devtools@8935

@tanstack/solid-query-persist-client

npm i https://pkg.pr.new/@tanstack/solid-query-persist-client@8935

@tanstack/solid-query

npm i https://pkg.pr.new/@tanstack/solid-query@8935

@tanstack/svelte-query

npm i https://pkg.pr.new/@tanstack/svelte-query@8935

@tanstack/svelte-query-devtools

npm i https://pkg.pr.new/@tanstack/svelte-query-devtools@8935

@tanstack/svelte-query-persist-client

npm i https://pkg.pr.new/@tanstack/svelte-query-persist-client@8935

@tanstack/vue-query

npm i https://pkg.pr.new/@tanstack/vue-query@8935

@tanstack/vue-query-devtools

npm i https://pkg.pr.new/@tanstack/vue-query-devtools@8935

commit: c3b2554

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 2, 2025

oh, that’s neat. However, we also use the paths for vitest via vite-tsconfig-paths. Is there a way to have vitest pick up those customConditions, or would this work per default and all we’d need to do is remove vite-tsconfig-paths ?

It also seems that publint doesn’t like the exports order:

pkg.exports["."].import.types should be the first in the object as required by TypeScript.

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 2, 2025

yeah vitest doesn’t seem to pick it up now:

Error: Failed to resolve entry for package "@tanstack/query-core". The package may have incorrect main/module/exports specified in its package.json.

I think we need to set:

  resolve: {
    conditions: ['tanstack-internal']
  }

https://github.com/vitejs/vite-ts-monorepo-rfc/blob/bc265396753de4326054dcbe4e538ad35a9b4554/with-exports-conditions/packages/app/vite.config.ts#L4-L6

and remove the vite-tsconfig-paths plugin

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 3, 2025

do I need to remove the dynamicAliases next?

yeah I guess they are no longer needed

@tylersayshi
Copy link
Contributor Author

hm, I'm pretty stumped with the current CI errors.

What's weird is that some packages pass without issue to resolve the internal references, while others fail.

Also, each of the failing things pass locally, so my new theory is that the ts files from each package needs to be copied during the setup step of nx maybe? Or something along those lines?

@tylersayshi tylersayshi marked this pull request as ready for review April 4, 2025 16:48
@tylersayshi
Copy link
Contributor Author

marking this ready - seems like CI does indeed pass now 🙂

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 4, 2025

Love it 😍

@TkDodo TkDodo merged commit 8e0c6b7 into TanStack:main Apr 4, 2025
5 checks passed
@TkDodo
Copy link
Collaborator

TkDodo commented Apr 4, 2025

hmm, it fail on main after the merge, too:

https://github.com/TanStack/query/actions/runs/14272681841

could it be order dependent, or maybe caching related ? Maybe there’s an additional output by typescript that needs to be cached? nx distributes the runs over multiple machines, so maybe something is missing on some of them ...

@tylersayshi
Copy link
Contributor Author

tylersayshi commented Apr 4, 2025

could it be order dependent, or maybe caching related ? Maybe there’s an additional output by typescript that needs to be cached? nx distributes the runs over multiple machines, so maybe something is missing on some of them ...

huh, ok. This should probably be reverted for now then.

Is there a way to disable nx cache to run the tests locally?

edit: I found that there is a way, I'll try this: https://nx.dev/recipes/running-tasks/skipping-cache

@tylersayshi
Copy link
Contributor Author

could it be order dependent

I think this is the case. When run locally, query-core compiles first, which would make it available, where in the CI it shows as not found

image

nx is supposed to understand dependency order, so this seems strange that it could run out of order though

nrwl/nx#6577 (comment)

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 5, 2025

looking at the build output it definitely runs before:

https://github.com/TanStack/query/actions/runs/14272681841/job/40009248769

Screenshot 2025-04-05 at 17 41 24

so, I really don’t know. We also had everything green on the PR.

@tylersayshi
Copy link
Contributor Author

hmm I think it has to do with the nx cache. From the run you sent above: https://cloud.nx.app/runs/OFYNaHzapT

image

the cache misses are almost all fails (maybe the cache hits are stale and missing the customCondition changes?). And the "Remote cache hit" is all success.

Do you have access on nx to rerun the CI with disabling the remote cache just to see if that helps?

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 5, 2025

let’s try with --skip-nx-cache on the task that runs in CI:

3319d35

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 5, 2025

yeah that worked; I hope I find a way to purge the remote cache

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 5, 2025

I changed nx.json to get complete cache invalidation, and even with all cache misses, it still fails. I don’t think there’s something wrongly cached, I think it’s more related with distributed execution. If compile compile something and creates an output, but test runs on a different machine that doesn’t get access to what compile has produced, that would imo explain the behaviour.

but why was it working before and not now? are outputs still written to dist-ts like they were before? Because that’s what compile caches:

query/nx.json

Line 31 in 57d159c

"outputs": ["{projectRoot}/dist-ts"]

@tylersayshi
Copy link
Contributor Author

So the fails are specific to vitest... This is a shot in the dark, but what if we exclude resolve.conditions when process.env.CI is true? The build output from compile should be usable at that point, so it should be safe to do.

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 5, 2025

it also fails on test:types, I don’t think its vitest specific, because why would it work locally then?

They purged the cache for us, and then it almost went through with just 3 pending tasks, but something else failed. I re-started, and now 167 tasks came from cache, and we have a failure again:

https://cloud.nx.app/runs/Wu8RdNSBRi

So my thinking is still that we are not persisting everything in the cache that is needed, but I don’t know what’s missing

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 5, 2025

okay, I reproduced it locally:

  • pnpn rum clean to clear everything
  • npx nx run @tanstack/query-core:compile that puts the output into dist-ts and also caches it
  • npx nx run @tanstack/react-query:test:lib now fails with
Error: Failed to resolve entry for package "@tanstack/query-core". The package may have incorrect main/module/exports specified in its package.json.

so maybe it’s not nx related after all? It feels like compile is not producing what typescript needs to resolve the references?

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 5, 2025

only if I run:

npx nx run @tanstack/query-core:build

the tests start to work!

but that is not a dependency we have defined in nx, so it’s clear why it works sometimes and mostly doesn’t.

now the real question is: why does vite needs a build before it can resolve those? As a quick workaround, we could make the test task depend on the build task (instead of the compile task), but since we build with tsup I’d rather avoid this...

tylersayshi added a commit to tylersayshi/tanstack-query that referenced this pull request Apr 8, 2025
TkDodo added a commit that referenced this pull request Apr 9, 2025
#8973)

* Revert "chore(paths): use customConditions to define internal imports (#8935)"

This reverts commit 8e0c6b7.

* fix export

* refactor: make test:lib depend on compile again

---------

Co-authored-by: Dominik Dorfmeister <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment