-
Notifications
You must be signed in to change notification settings - Fork 338
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
Add react-router v7 example #752
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@chrisvxd deps are failing to install because we need to bump the minimum engine version for node:
|
27e1224
to
62ddbcc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome thanks @matthewlynch! Left some comments 🙃
.github/workflows/ci.yml
Outdated
@@ -19,7 +19,7 @@ jobs: | |||
- name: Setup Node.js | |||
uses: actions/setup-node@v3 | |||
with: | |||
node-version: 18 | |||
node-version: 20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bump is causing CI to fail for Vercel. Noted your comment on RR7 dependencies, though. Need to probe this a bit further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could replicate the issue locally, moving to v22 (LTS) works so will try on CI once I've sorted a monorepo issue.
|
||
import "@measured/puck/puck.css"; | ||
|
||
import type { Route } from "./+types/puck-splat"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this +
import magic?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something new in RR7. Types are auto generated based on file changes for the route. Works really well. See https://reactrouter.com/how-to/route-module-type-safety.
@@ -0,0 +1,13 @@ | |||
export function resolvePuckPath(path: string = "") { | |||
const url = new URL(path, "https://placeholder.com/"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear that placeholder.com
doesn't need to be changed by the user (in fact, I thought I would have to change it after generating).
I understand the technique here, but is there a cleaner way we could do this without requiring the placeholder string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a comment saying it isn't important and set the origin as a default parameter value. Let me know if you want to change it to something else.
recipes/react-router/package.json
Outdated
"@measured/puck": "^0.17.1", | ||
"@react-router/node": "^7.0.2", | ||
"@react-router/serve": "^7.0.2", | ||
"isbot": "^5.1.17", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed this is in the standard react-router template, but is this dep necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defaults from the base RR template, it can be removed but we'd need to add a new server entry file because RR expects the dependency to be there. I think it should be kept based on: https://react.dev/reference/react-dom/server/renderToPipeableStream#waiting-for-all-content-to-load-for-crawlers-and-static-generation.
Let me know what you think? For now, I've run react-router reveal
which generates the default files that is being used.
This PR adds a react-router recipe for Puck.
Note: I had to bump the node version to
22
because some of the RR7 deps required a minimum version of 20 and that caused issues with some of the next js apps.