-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Conversation
View your CI Pipeline Execution ↗ for commit c3b2554.
☁️ Nx Cloud last updated this comment at |
oh, that’s neat. However, we also use the paths for It also seems that publint doesn’t like the exports order:
|
yeah vitest doesn’t seem to pick it up now:
I think we need to set:
and remove the |
yeah I guess they are no longer needed |
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? |
marking this ready - seems like CI does indeed pass now 🙂 |
Love it 😍 |
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 ... |
huh, ok. This should probably be reverted for now then.
edit: I found that there is a way, I'll try this: https://nx.dev/recipes/running-tasks/skipping-cache |
looking at the build output it definitely runs before: https://github.com/TanStack/query/actions/runs/14272681841/job/40009248769 so, I really don’t know. We also had everything green on the PR. |
hmm I think it has to do with the nx cache. From the run you sent above: https://cloud.nx.app/runs/OFYNaHzapT ![]() 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? |
let’s try with |
yeah that worked; I hope I find a way to purge the remote cache |
I changed but why was it working before and not now? are outputs still written to Line 31 in 57d159c
|
So the fails are specific to |
it also fails on test:types, I don’t think its 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 |
okay, I reproduced it locally:
so maybe it’s not nx related after all? It feels like |
only if I run:
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... |
…TanStack#8935)" This reverts commit 8e0c6b7.
#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]>
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-monorepofixes #8865