Make react router as peer dep instead of dependency #3001
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
getting latest main
☁️ Nx Cloud ReportCI is running/has finished running commands for commit e98f51c. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 2 targetsSent with 💌 from NxCloud. |
|
start cannot be used without router, so a peer dependency is not expressing the correct relationship. |
|
May be we can expose all functions like createRouter from start then? By reexporting I mean. |
|
this seems like a workaround for some bundler issues. what's the proper solution here? |
| @@ -134,7 +134,6 @@ | |||
| }, | |||
| "dependencies": { | |||
| "@tanstack/react-cross-context": "workspace:^", | |||
There was a problem hiding this comment.
It may be worth checking out, whether we are actually using the react-cross-context package and possibly remove it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I came across this while debugging #3002
Some package managers/resolvers in certain cases, load different downstream packages for different imports.
For instance, in tanstack/start quick starter app, we import both
@tanstack/startand@tanstack/router.@tanstack/routeris imported in user application(See Getting Started) as well in@tanstack/start. In one place the resolver may resolve to a different module than in the other places.deno, for instance resolves
@tanstack/react-routerin user application to node_modules/.deno/@TanStack[email protected]_1 and@tanstack/react-routerin@tanstack/startto node_modules/.deno/@TanStack[email protected]This causes a null router value to be returned by useRouterState() and the following logs:
when the
<Outlet />code ultimately fetches context.In other words, the module that hung the context up was in
ssr.tsxviacreateStartHandler/StartServerwasnode_modules/.deno/@[email protected]_1(because resolver resolved to that module)While, the code uses(and gets from resolver)
node_modules/.deno/@[email protected]pnpm/deno/bun/yarn all three can resolve to different modules this way.
Related issues:
#3002 - deno issue
#2594 - refers to bun and pnpm
#2540 - refers to pnpm and yarn
#1692 - doesn't mention the package manager used, but seems to indicate a similar issue.
I have fixed only the react-router issue for now. But if people used
@tanstack/react-cross-contextdirectly in their code, then they will see this issue there as well.This means that potentially all the libraries in tanstack start/router should be appropriately set as peer dependencies instead of dependencies as a matter of best practice(because unlike a typical monorepo tanstack router packages should be independently usable as well). I will leave it for @tannerlinsley if/when he wants to do these changes, if he accepts this fix.
At the very least,
@tanstack/react-cross-contextshould be declared as peer dependency in@tanstack/routeras it is the container where we hang up the context(a singleton). Otherwise people will use it in their code and end up with two different contexts and will end up hitting their heads against the wall on why their context is empty in some part of the code while it is correctly obtained in other places(or worse they will have some 10 contexts).The potential packages can be seen by dependency tree where this practice should apply:
BTW this issue can show up in node as well if npm installs nested dependencies. I am not 100% sure on the module resolution logic of current npm but I believe at least older npm versions used to do this.