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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: miho-react-router-7-poc
Choose a base branch
from
Open

[WIP] Brought back APIs, moved route generation to proper v7 convention, allowed for user decision on SSR #2449

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5480034
Brought back APIs, moved route generation to proper v7 convention
AlemTuzlak Jan 11, 2025
b07b5be
fixed up storage to be more generic
AlemTuzlak Jan 11, 2025
05e3fda
SSR decision
AlemTuzlak Jan 11, 2025
999775e
removed extra file
AlemTuzlak Jan 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions waspc/data/Generator/templates/react-app/src/catchall.tsx
View file
Open in desktop

This file was deleted.

6 changes: 5 additions & 1 deletion waspc/data/Generator/templates/react-app/src/root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
Scripts,
ScrollRestoration,
} from "react-router";
import { DefaultRootErrorBoundary } from "./components/DefaultRootErrorBoundary";

export function Layout({
children,
Expand Down Expand Up @@ -35,6 +36,9 @@ export function Layout({
);
}


export default function Root() {
return <Outlet />;
}
}

export const ErrorBoundary = DefaultRootErrorBoundary
Copy link
Author

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

57 changes: 0 additions & 57 deletions waspc/data/Generator/templates/react-app/src/router.tsx
View file
Open in desktop
Copy link
Author

Choose a reason for hiding this comment

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

removed in favor of routes.ts

This file was deleted.

30 changes: 28 additions & 2 deletions waspc/data/Generator/templates/react-app/src/routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Author

Choose a reason for hiding this comment

The 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 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

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
Copy link
Author

Choose a reason for hiding this comment

The 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

Copy link
Author

@AlemTuzlak AlemTuzlak Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The 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:

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

] satisfies RouteConfig;
4 changes: 2 additions & 2 deletions waspc/data/Generator/templates/react-app/vite.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -16,7 +16,7 @@ const _waspUserProvidedConfig = {};

const defaultViteConfig = {
base: "{= baseDir =}",
plugins: [reactRouter()],
plugins: [reactRouterDevtools(), reactRouter()],
Copy link
Author

Choose a reason for hiding this comment

The 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']
},
Expand Down
52 changes: 27 additions & 25 deletions waspc/data/Generator/templates/sdk/wasp/api/index.ts
View file
Open in desktop
Copy link
Author

Choose a reason for hiding this comment

The 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:

  1. Add two storages (session storage on the server, the same storage you've been using up until now for the client)
  2. Make the code understand the operation context and fetch from the correct storage.

Copy link
Author

Choose a reason for hiding this comment

The 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
Expand Up @@ -11,7 +11,7 @@ export const api: AxiosInstance = axios.create({

const WASP_APP_AUTH_SESSION_ID_NAME = 'sessionId'

let waspAppAuthSessionId: string | undefined = undefined // storage.get(WASP_APP_AUTH_SESSION_ID_NAME) as string | undefined
let waspAppAuthSessionId: string | undefined = typeof window !== "undefined" ? storage.get(WASP_APP_AUTH_SESSION_ID_NAME) : undefined

// PRIVATE API (sdk)
export function setSessionId(sessionId: string): void {
Expand Down Expand Up @@ -39,37 +39,39 @@ export function removeLocalUserData(): void {
apiEventsEmitter.emit('sessionId.clear')
}

// api.interceptors.request.use((request) => {
// const sessionId = getSessionId()
// if (sessionId) {
// request.headers['Authorization'] = `Bearer ${sessionId}`
// }
// return request
// })
api.interceptors.request.use((request) => {
const sessionId = getSessionId()
if (sessionId) {
request.headers['Authorization'] = `Bearer ${sessionId}`
}
return request
})

// api.interceptors.response.use(undefined, (error) => {
// if (error.response?.status === 401) {
// clearSessionId()
// }
// return Promise.reject(error)
// })
api.interceptors.response.use(undefined, (error) => {
if (error.response?.status === 401) {
clearSessionId()
}
return Promise.reject(error)
})

// This handler will run on other tabs (not the active one calling API functions),
// and will ensure they know about auth session ID changes.
// Ref: https://developer.mozilla.org/en-US/docs/Web/API/Window/storage_event
// "Note: This won't work on the same page that is making the changes — it is really a way
// for other pages on the domain using the storage to sync any changes that are made."
// window.addEventListener('storage', (event) => {
// if (event.key === storage.getPrefixedKey(WASP_APP_AUTH_SESSION_ID_NAME)) {
// if (!!event.newValue) {
// waspAppAuthSessionId = event.newValue
// apiEventsEmitter.emit('sessionId.set')
// } else {
// waspAppAuthSessionId = undefined
// apiEventsEmitter.emit('sessionId.clear')
// }
// }
// })
if (typeof window !== 'undefined') {
window.addEventListener('storage', (event) => {
if (event.key === storage.getPrefixedKey(WASP_APP_AUTH_SESSION_ID_NAME)) {
if (!!event.newValue) {
waspAppAuthSessionId = event.newValue
apiEventsEmitter.emit('sessionId.set')
} else {
waspAppAuthSessionId = undefined
apiEventsEmitter.emit('sessionId.clear')
}
}
})
}

// PRIVATE API (sdk)
/**
Expand Down
2 changes: 1 addition & 1 deletion waspc/data/Generator/templates/sdk/wasp/core/storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ function createLocalStorageDataStore(prefix: string): DataStore {
export const storage = createLocalStorageDataStore('wasp')

function ensureLocalStorageIsAvailable(): void {
if (!window || !window.localStorage) {
if (typeof window === 'undefined' || !window.localStorage) {
throw new Error('Local storage is not available.')
}
}
2 changes: 2 additions & 0 deletions waspc/src/Wasp/Generator/WebAppGenerator.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,8 @@ npmDepsForWasp _spec =
-- NOTE: Make sure to bump the version of the tsconfig
-- when updating Vite or React versions
("@tsconfig/vite-react", "^2.0.0"),
-- NOTE: You can remove this if you don't want to use the React Router DevTools
("react-router-devtools", "^1.1.0"),
("@react-router/dev", "^7.1.1")
]
}
Expand Down