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

Type-safe href (#12994) #13012

Merged
merged 2 commits into from
Feb 12, 2025
Merged

Type-safe href (#12994) #13012

merged 2 commits into from
Feb 12, 2025

Conversation

pcattori
Copy link
Contributor

  • wip: type-safe href

  • consistent params parsing + type generation

  • href tests

  • href typegen tests

  • href types normalize route full path

  • fix react-router --version

The --version flag reads the local package.json at ../package.json. While this path is correct when running from source, it is incorrect after the CLI is built since package.json stays at the root of the package, but the built code gets nested into dist/.

I only noticed this discrepancy because I was converting the unit tests to integration tests to fix an incompatibility issue with Node v22.14 and esbuild-register.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
* wip: type-safe href

* consistent params parsing + type generation

* href tests

* href typegen tests

* href types normalize route full path

* fix `react-router --version`

The `--version` flag reads the local `package.json`
at `../package.json`. While this path is correct when running from
source, it is incorrect after the CLI is built since `package.json`
stays at the root of the package, but the built code gets nested into
`dist/`.

I only noticed this discrepancy because I was converting the unit tests
to integration tests to fix an incompatibility issue with Node v22.14 and
`esbuild-register`.
Copy link

changeset-bot bot commented Feb 12, 2025

🦋 Changeset detected

Latest commit: b86e8c0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages
Name Type
@react-router/dev Major
react-router Major
@react-router/fs-routes Major
@react-router/remix-routes-option-adapter Major
@react-router/architect Major
@react-router/cloudflare Major
react-router-dom Major
@react-router/express Major
@react-router/node Major
@react-router/serve Major
create-react-router Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -0,0 +1,174 @@
import { spawnSync } from "node:child_process";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍 Awesome to have tests for this now!

@pcattori pcattori merged commit 1b7b6b8 into release-next Feb 12, 2025
8 checks passed
@pcattori pcattori deleted the pedro/release-type-safe-href branch February 12, 2025 19:25
Copy link
Contributor

🤖 Hello there,

We just published version 7.2.0-pre.1 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 7.2.0-pre.2 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 7.2.0-pre.5 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 7.2.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@ngbrown ngbrown mentioned this pull request Feb 20, 2025
3 tasks
@skovhus
Copy link

skovhus commented Feb 22, 2025

I haven’t looked too much into the code here, but I’m curious if it would be possible to utilize this feature in library mode (I’m fine doing some custom wiring on our end). Any recommendations would be much appreciated.

Copy link
Contributor

github-actions bot commented Mar 6, 2025

🤖 Hello there,

We just published version 7.3.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@skovhus
Copy link

skovhus commented Mar 7, 2025

@pcattori any suggestions if this could be in library mode?

I haven’t looked too much into the code here, but I’m curious if it would be possible to utilize this feature in library mode (I’m fine doing some custom wiring on our end). Any recommendations would be much appreciated.

@pcattori
Copy link
Contributor Author

pcattori commented Mar 7, 2025

@skovhus all new type-safety features assume you are using routes.ts to define routes and that each route is implemented as a route module (with exports like loader, default, etc.). Currently those features are only available in framework mode and we don't have short-term plans to bring those features to library mode. We also aren't ruling that out either, but yea not planned as of today

That said, you can already build a SPA in framework mode today, so if you really want those type features but don't want SSR and all the other stuff, then SPA mode is probably your best bet.

@skovhus
Copy link

skovhus commented Mar 7, 2025

@pcattori Thanks for the response. We have currently rolled our own system on top of library mode, but I do like the features you are baking into framework mode.

I’m really curious if clientLoader are split from the lazy loaded route module file. Like are they loaded immediately before the component is loaded from the module file? That something we do right now that means we can load and hydrate data while the view JS is loading.

@pcattori
Copy link
Contributor Author

@skovhus that's probably a better question for our Discord

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

Successfully merging this pull request may close these issues.

None yet

3 participants