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

refactor(router-core): moving the router core into separate package #3171

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

birkskyum
Copy link

@birkskyum birkskyum commented Jan 14, 2025

This is a preparation step for:

It moves pieces of shared logic from react-router to router-core, and imports it from there.

The exports in react-router/src/index.tsx are kept as is, by re-exporting functions from router-core, to avoid breaking changes.

@birkskyum birkskyum force-pushed the router-core branch 2 times, most recently from 1a7588d to 780faf6 Compare January 14, 2025 23:17
@birkskyum birkskyum changed the title extract parts of react-router to router-core Extract parts of react-router to an agnostic router-core package Jan 14, 2025
@birkskyum birkskyum changed the title Extract parts of react-router to an agnostic router-core package Extract parts of react-router to shared router-core package Jan 14, 2025
SeanCassiere

This comment was marked as resolved.

@SeanCassiere SeanCassiere marked this pull request as draft January 15, 2025 00:07
@SeanCassiere

This comment was marked as resolved.

@birkskyum

This comment was marked as outdated.

Copy link

nx-cloud bot commented Jan 16, 2025

View your CI Pipeline Execution ↗ for commit b6eafc7.

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ✅ Succeeded 5m 12s View ↗

☁️ Nx Cloud last updated this comment at 2025-01-23 07:57:17 UTC

Copy link

pkg-pr-new bot commented Jan 16, 2025

Open in Stackblitz

More templates

@tanstack/directive-functions-plugin

npm i https://pkg.pr.new/@tanstack/directive-functions-plugin@3171

@tanstack/arktype-adapter

npm i https://pkg.pr.new/@tanstack/arktype-adapter@3171

@tanstack/eslint-plugin-router

npm i https://pkg.pr.new/@tanstack/eslint-plugin-router@3171

@tanstack/react-cross-context

npm i https://pkg.pr.new/@tanstack/react-cross-context@3171

@tanstack/history

npm i https://pkg.pr.new/@tanstack/history@3171

@tanstack/react-router

npm i https://pkg.pr.new/@tanstack/react-router@3171

@tanstack/create-router

npm i https://pkg.pr.new/@tanstack/create-router@3171

@tanstack/react-router-with-query

npm i https://pkg.pr.new/@tanstack/react-router-with-query@3171

@tanstack/router-cli

npm i https://pkg.pr.new/@tanstack/router-cli@3171

@tanstack/create-start

npm i https://pkg.pr.new/@tanstack/create-start@3171

@tanstack/router-core

npm i https://pkg.pr.new/@tanstack/router-core@3171

@tanstack/router-devtools

npm i https://pkg.pr.new/@tanstack/router-devtools@3171

@tanstack/router-generator

npm i https://pkg.pr.new/@tanstack/router-generator@3171

@tanstack/router-plugin

npm i https://pkg.pr.new/@tanstack/router-plugin@3171

@tanstack/router-vite-plugin

npm i https://pkg.pr.new/@tanstack/router-vite-plugin@3171

@tanstack/server-functions-plugin

npm i https://pkg.pr.new/@tanstack/server-functions-plugin@3171

@tanstack/start

npm i https://pkg.pr.new/@tanstack/start@3171

@tanstack/start-api-routes

npm i https://pkg.pr.new/@tanstack/start-api-routes@3171

@tanstack/start-client

npm i https://pkg.pr.new/@tanstack/start-client@3171

@tanstack/start-config

npm i https://pkg.pr.new/@tanstack/start-config@3171

@tanstack/start-plugin

npm i https://pkg.pr.new/@tanstack/start-plugin@3171

@tanstack/start-router-manifest

npm i https://pkg.pr.new/@tanstack/start-router-manifest@3171

@tanstack/start-server

npm i https://pkg.pr.new/@tanstack/start-server@3171

@tanstack/start-server-functions-client

npm i https://pkg.pr.new/@tanstack/start-server-functions-client@3171

@tanstack/start-server-functions-fetcher

npm i https://pkg.pr.new/@tanstack/start-server-functions-fetcher@3171

@tanstack/start-server-functions-handler

npm i https://pkg.pr.new/@tanstack/start-server-functions-handler@3171

@tanstack/start-server-functions-server

npm i https://pkg.pr.new/@tanstack/start-server-functions-server@3171

@tanstack/start-server-functions-ssr

npm i https://pkg.pr.new/@tanstack/start-server-functions-ssr@3171

@tanstack/valibot-adapter

npm i https://pkg.pr.new/@tanstack/valibot-adapter@3171

@tanstack/virtual-file-routes

npm i https://pkg.pr.new/@tanstack/virtual-file-routes@3171

@tanstack/zod-adapter

npm i https://pkg.pr.new/@tanstack/zod-adapter@3171

commit: b6eafc7

@birkskyum

This comment was marked as resolved.

@SeanCassiere

This comment was marked as resolved.

@SeanCassiere SeanCassiere changed the title Extract parts of react-router to shared router-core package refactor(router-core): moving the router core into separate package Jan 16, 2025
@birkskyum

This comment was marked as resolved.

@SeanCassiere

This comment was marked as resolved.

@birkskyum

This comment was marked as resolved.

@SeanCassiere

This comment was marked as resolved.

@birkskyum birkskyum force-pushed the router-core branch 3 times, most recently from cf8a80c to fb5875e Compare January 18, 2025 12:47
@birkskyum
Copy link
Author

I think basically we're facing a hoisting issue, similar to:

@birkskyum

This comment was marked as resolved.

@birkskyum
Copy link
Author

birkskyum commented Jan 22, 2025

As stated here:

Generated types are not fully functional without hoisted node_modules

Meaning that users can have partial types (the @tanstack/router-core won't be resolved), or add public-hoist-pattern[]=@tanstack/router-core to .npmrc

It's unlikely I'll find a better solution for hoisting a nested package than the Nuxt/UnJS team who deep dived this.

The solutions I see here are basically to treat router-core not as a package, but just as a simple folder that's referenced or otherwise copied into the react-router/solid-router packages. Or alternatively, to duplicate this code for now, and abstract it out at next major release with a recommendation to install @tanstack/router-core

@SeanCassiere
Copy link
Member

@birkskyum thanks for your work on this one. I've pushed this to the team for review and we should be able to merge tomorrow.


In response to your comment above, we'll wait to see the common practice going forward since this pattern is currently in use by TanStack Table as well, so we'll be to apply whatever changes to the TanStack ecosystem as whole when the "correct" way has been figured out.

@birkskyum
Copy link
Author

birkskyum commented Jan 23, 2025

Sounds good. And the CI just passed? Did the tsconfig change do the trick, making react-router a self-contained module/folder? or does it run fewer tests

@SeanCassiere
Copy link
Member

... And the CI just passed?

One of your commits prior to 538b8b9 did the trick. All that remains were eslint errors.

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.

2 participants