[WIP] Brought back APIs, moved route generation to proper v7 convention, allowed for user decision on SSR #2449
Conversation
| import { router } from './router' | ||
|
|
||
| export default function Component() { | ||
| return router; |
There was a problem hiding this comment.
the reason this was removed was due to the fact that you would create a browserRouter inside the already existing react-router router. I am not sure if this worked before, I'd expect it to not work due to nested routers but maybe it did.
There was a problem hiding this comment.
entry.client.ts creates the HydratedRouter and wraps the whole app with it
| } | ||
|
|
||
| export const ErrorBoundary = DefaultRootErrorBoundary |
There was a problem hiding this comment.
brought back the error boundary for the whole app
There was a problem hiding this comment.
removed in favor of routes.ts
| {=/ isExternalAuthEnabled =} | ||
| ] | ||
| const userDefinedRoutes = Object.entries(routes).map(([routeKey, route]) => { | ||
| // TODO: This should be the path to the route module file (eg. /src/routes/home.tsx) |
There was a problem hiding this comment.
I would need a little bit of help here, the current api of the route function is to expect the url segment as the first arg and the path to the module as the second, so eg:
route("login", "./app/login.tsx")Not sure how you generate routes but I'm pretty sure you have to have a path to the actual route, all you have to do is replace the route argument with the actual correct thing, so just adapt
export const routeNameToRouteComponent = {
{=# routes =}
{= name =}: {= targetComponent =},
{=/ routes =}
} as const;to actually work with modules I guess
| @@ -16,7 +16,7 @@ const _waspUserProvidedConfig = {}; | |||
|
|
|||
| const defaultViteConfig = { | |||
| base: "{= baseDir =}", | |||
| plugins: [reactRouter()], | |||
| plugins: [reactRouterDevtools(), reactRouter()], | |||
There was a problem hiding this comment.
I sneakily added in dev tools for react-router, don't tell the contributors! Jokes aside, remove if you don't like it, or think it's useless
There was a problem hiding this comment.
so this file is a bit suspicious in terms of what it should look like, I see you use axios, which is perfectly fine, but using browser APIs locks you into drumroll client-side rendering.
I would need to understand what exactly is the scope of this functionality to help out and change it, from what I see it's supposed to handle authentication. If that's the case and you want to support both SSR and CSR you will need to do the following:
- Add two storages (session storage on the server, the same storage you've been using up until now for the client)
- Make the code understand the operation context and fetch from the correct storage.
There was a problem hiding this comment.
After looking at this for a second I realise there's no need to change this file, but rather the storage interface, I'll elaborate more there
| @@ -6,45 +6,50 @@ export type DataStore = { | |||
| clear(): void | |||
| } | |||
|
|
|||
| function createLocalStorageDataStore(prefix: string): DataStore { | |||
| function createStorageDataStore(prefix: string): DataStore { | |||
There was a problem hiding this comment.
I made this generic as if you want to support SSR you'll need to not rely on web api's
| function ensureLocalStorageIsAvailable(): void { | ||
| if (!window || !window.localStorage) { | ||
| throw new Error('Local storage is not available.') | ||
| // TODO: Make this function work in the server context as well. |
There was a problem hiding this comment.
this will be a bit trickier, if you want to use the react-router built in session handling it would require the request object as well which would have to be piped to here. Up for discussion with you guys what you'd want to do
| {=# ssr.isDefined =} | ||
| isSSR = Boolean("{= ssr.value =}") | ||
| {=/ ssr.isDefined =}, | ||
|
|
||
| export default { | ||
| appDirectory: "src", | ||
| ssr: false, | ||
| ssr: isSSR, |
There was a problem hiding this comment.
I'm just a good ol' JS dev, I don't speak in haskell, but you'll get the idea
| layout( | ||
| // TODO: This should maybe always be defined, if not, | ||
| // TODO: the code should be (if defined => layout, otherwise => spread the routes normally in the array) | ||
| {=# rootComponent.isDefined =} | ||
| "{= rootComponent.importIdentifier =}" | ||
| {=/ rootComponent.isDefined =}, | ||
| [...waspDefinedRoutes, ...userDefinedRoutes] | ||
| ), |
There was a problem hiding this comment.
brought back the functionality from the router.ts but the issue is that layout needs to be defined, so it will either be always defined OR:
isDefined ? layout(path, children) : childrenThere was a problem hiding this comment.
also, another cool this opens up as a possibility for you guys is to add layout routes etc as functionalities in wasp, eg in your wasp config you could do something like:
layout RootLayout { path: "/", to: MainPage }
route RootRoute { path: "/", to: MainPage, layout: RootLayout }
This would increase complexity obviously on your side, but would also make users happy, I'd assume
Description
Continuation of moving to react-router v7 PoC with some improvements, notably:
Select what type of change this PR introduces:
Update Waspc ChangeLog and version if needed
If you did a bug fix, new feature, or breaking change, that affects waspc, make sure you satisfy the following:
Update example apps if needed
If you did code changes and added a new feature or modified an existing feature, make sure you satisfy the following:
waspc/examples/todoAppas needed (updated modified feature or added new feature) and manually checked it works correctly.waspc/headless-test/examples/todoAppand its e2e tests as needed (updated modified feature and its tests or added new feature and new tests for it).