-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[WIP] Brought back APIs, moved route generation to proper v7 convention, allowed for user decision on SSR #2449
base: miho-react-router-7-poc
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed in favor of |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,8 +2,34 @@ import { | |
type RouteConfig, | ||
route, | ||
} from "@react-router/dev/routes"; | ||
import { layout, route } from "@react-router/dev/routes"; | ||
import { routes } from 'wasp/client/router' | ||
|
||
export const routeNameToRouteComponent = { | ||
{=# routes =} | ||
{= name =}: {= targetComponent =}, | ||
{=/ routes =} | ||
} as const; | ||
|
||
const waspDefinedRoutes = [ | ||
{=# isExternalAuthEnabled =} | ||
route("{= oAuthCallbackPath =}", "./auth/pages/OAuthCallback"), | ||
{=/ 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would need a little bit of help here, the current api of the 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 export const routeNameToRouteComponent = {
{=# routes =}
{= name =}: {= targetComponent =},
{=/ routes =}
} as const; to actually work with modules I guess |
||
return route(routeKey, route) | ||
|
||
}) | ||
|
||
|
||
export default [ | ||
// * matches all URLs, the ? makes it optional so it will match / as well | ||
route("*?", "catchall.tsx"), | ||
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] | ||
), | ||
Comment on lines
+27
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) : children There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
This would increase complexity obviously on your side, but would also make users happy, I'd assume |
||
] satisfies RouteConfig; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ | |
import { mergeConfig } from "vite"; | ||
import { reactRouter } from "@react-router/dev/vite"; | ||
import { defaultExclude } from "vitest/config" | ||
|
||
import { reactRouterDevtools } from "react-router-devtools"; | ||
{=# customViteConfig.isDefined =} | ||
// Ignoring the TS error because we are importing a file outside of TS root dir. | ||
// @ts-ignore | ||
|
@@ -16,7 +16,7 @@ const _waspUserProvidedConfig = {}; | |
|
||
const defaultViteConfig = { | ||
base: "{= baseDir =}", | ||
plugins: [reactRouter()], | ||
plugins: [reactRouterDevtools(), reactRouter()], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
optimizeDeps: { | ||
exclude: ['wasp'] | ||
}, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made this generic as if you want to support SSR you'll need to not rely on web api's |
||
function getPrefixedKey(key: string): string { | ||
return `${prefix}:${key}` | ||
} | ||
|
||
return { | ||
getPrefixedKey, | ||
set(key, value) { | ||
ensureLocalStorageIsAvailable() | ||
localStorage.setItem(getPrefixedKey(key), JSON.stringify(value)) | ||
const storage =getStorage() | ||
storage?.setItem(getPrefixedKey(key), JSON.stringify(value)) | ||
}, | ||
get(key) { | ||
ensureLocalStorageIsAvailable() | ||
const value = localStorage.getItem(getPrefixedKey(key)) | ||
const storage = getStorage() | ||
const value = storage?.getItem(getPrefixedKey(key)) | ||
try { | ||
return value ? JSON.parse(value) : undefined | ||
} catch (e: any) { | ||
return undefined | ||
} | ||
}, | ||
remove(key) { | ||
ensureLocalStorageIsAvailable() | ||
localStorage.removeItem(getPrefixedKey(key)) | ||
const storage = getStorage() | ||
storage?.removeItem(getPrefixedKey(key)) | ||
}, | ||
clear() { | ||
ensureLocalStorageIsAvailable() | ||
Object.keys(localStorage).forEach((key) => { | ||
const storage = getStorage() | ||
if(!storage) { | ||
return | ||
} | ||
Object.keys(storage).forEach((key) => { | ||
if (key.startsWith(prefix)) { | ||
localStorage.removeItem(key) | ||
storage.removeItem(key) | ||
} | ||
}) | ||
}, | ||
} | ||
} | ||
|
||
export const storage = createLocalStorageDataStore('wasp') | ||
export const storage = createStorageDataStore('wasp') | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
function getStorage(): Storage | undefined { | ||
if (typeof localStorage === 'undefined') { | ||
return undefined | ||
} | ||
return localStorage | ||
} |
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.
brought back the error boundary for the whole app