From 0ca221cb0312d07388c798fe75bc1a1d101bb786 Mon Sep 17 00:00:00 2001 From: Daniel Choudhury Date: Fri, 4 Aug 2023 14:25:09 +0700 Subject: [PATCH 1/4] fix(router): Prevent rerendering authenticated routes on hash change (#9007) --- packages/router/src/AuthenticatedRoute.tsx | 5 +++-- packages/router/src/__tests__/router.test.tsx | 11 +++++++++-- packages/router/src/router.tsx | 18 ++++++++++++------ packages/router/src/util.ts | 5 +++-- 4 files changed, 27 insertions(+), 12 deletions(-) diff --git a/packages/router/src/AuthenticatedRoute.tsx b/packages/router/src/AuthenticatedRoute.tsx index dbac615d2446..a3bdba1726d5 100644 --- a/packages/router/src/AuthenticatedRoute.tsx +++ b/packages/router/src/AuthenticatedRoute.tsx @@ -12,8 +12,9 @@ interface AuthenticatedRouteProps { whileLoadingAuth?: () => React.ReactElement | null private?: boolean } - -export function AuthenticatedRoute(props: AuthenticatedRouteProps) { +export const AuthenticatedRoute: React.FC = ( + props +) => { const { private: isPrivate, unauthenticated, diff --git a/packages/router/src/__tests__/router.test.tsx b/packages/router/src/__tests__/router.test.tsx index b81709992c7e..acb1116fa870 100644 --- a/packages/router/src/__tests__/router.test.tsx +++ b/packages/router/src/__tests__/router.test.tsx @@ -36,7 +36,7 @@ import { Route, Private, Redirect, - routes, + routes as generatedRoutes, Link, navigate, back, @@ -45,7 +45,7 @@ import { import { useLocation } from '../location' import { useParams } from '../params' import { Set } from '../Set' -import { Spec } from '../util' +import type { Spec, GeneratedRoutesMap } from '../util' /** running into intermittent test timeout behavior in https://github.com/redwoodjs/redwood/pull/4992 attempting to work around by bumping the default timeout of 5000 */ @@ -59,9 +59,16 @@ type UnknownAuthContextInterface = AuthContextInterface< unknown, unknown, unknown, + unknown, + unknown, + unknown, + unknown, unknown > +// The types are generated in the user's project +const routes = generatedRoutes as GeneratedRoutesMap + function createDummyAuthContextValues( partial: Partial ) { diff --git a/packages/router/src/router.tsx b/packages/router/src/router.tsx index 461a6497966f..e3934e82e088 100644 --- a/packages/router/src/router.tsx +++ b/packages/router/src/router.tsx @@ -27,6 +27,7 @@ import { TrailingSlashesTypes, validatePath, } from './util' +import type { Wrappers } from './util' import type { AvailableRoutes } from './index' @@ -203,7 +204,7 @@ const LocationAwareRouter: React.FC = ({ } interface WrappedPageProps { - wrappers: ReactNode[] + wrappers: Wrappers routeLoaderElement: ReactNode setProps: Record } @@ -220,6 +221,9 @@ interface WrappedPageProps { */ const WrappedPage = memo( ({ wrappers, routeLoaderElement, setProps }: WrappedPageProps) => { + // @NOTE: don't mutate the wrappers array, it causes full page re-renders + // Instead just create a new array with the AuthenticatedRoute wrapper + let wrappersWithAuthMaybe = wrappers if (setProps.private) { if (!setProps.unauthenticated) { throw new Error( @@ -227,12 +231,14 @@ const WrappedPage = memo( ) } - wrappers.unshift(AuthenticatedRoute as any) + wrappersWithAuthMaybe = [AuthenticatedRoute, ...wrappers] } - if (wrappers.length > 0) { - // If wrappers exist e.g. [a,b,c] -> - return wrappers.reduceRight((acc, wrapper) => { + if (wrappersWithAuthMaybe.length > 0) { + // If wrappers exist e.g. [a,b,c] -> and returns a single ReactNode + // Wrap AuthenticatedRoute this way, because if we mutate the wrappers array itself + // it causes full rerenders of the page + return wrappersWithAuthMaybe.reduceRight((acc, wrapper) => { return React.createElement( wrapper as any, { @@ -240,7 +246,7 @@ const WrappedPage = memo( }, acc ? acc : routeLoaderElement ) - }, undefined) as any + }, undefined as ReactNode) } return routeLoaderElement diff --git a/packages/router/src/util.ts b/packages/router/src/util.ts index 32081ec7c4b2..ebb2188e2fcf 100644 --- a/packages/router/src/util.ts +++ b/packages/router/src/util.ts @@ -452,13 +452,14 @@ export type GeneratedRoutesMap = { } type RoutePath = string +export type Wrappers = Array<(props: any) => ReactNode> interface AnalyzedRoute { path: RoutePath name: string | null whileLoadingPage?: WhileLoadingPage page: PageType | null redirect: string | null - wrappers: ReactNode[] + wrappers: Wrappers setProps: Record setId: number } @@ -476,7 +477,7 @@ export function analyzeRoutes( interface RecurseParams { nodes: ReturnType whileLoadingPageFromSet?: WhileLoadingPage - wrappersFromSet?: ReactNode[] + wrappersFromSet?: Wrappers // we don't know, or care about, what props users are passing down propsFromSet?: Record setId?: number From 2daf3df75299410a020d40b6b96071ffcfa8bdbf Mon Sep 17 00:00:00 2001 From: Daniel Choudhury Date: Fri, 4 Aug 2023 16:11:30 +0700 Subject: [PATCH 2/4] Fix type error --- packages/vite/src/runFeServer.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/vite/src/runFeServer.ts b/packages/vite/src/runFeServer.ts index 43a7dab664e8..a2e9eff85687 100644 --- a/packages/vite/src/runFeServer.ts +++ b/packages/vite/src/runFeServer.ts @@ -212,10 +212,10 @@ export async function runFeServer() { const pageWithJs = currentRoute.renderMode !== 'html' // @NOTE have to add slash so subpaths still pick up the right file const bootstrapModules = pageWithJs - ? [ + ? ([ '/' + indexEntry.file, currentRoute.bundle && '/' + currentRoute.bundle, - ].filter(Boolean) + ].filter(Boolean) as string[]) : undefined const isSeoCrawler = checkUaForSeoCrawler(req.get('user-agent')) From d93f29a4501c085f4d8f763aadb2c9122c34a9e3 Mon Sep 17 00:00:00 2001 From: Daniel Choudhury Date: Fri, 4 Aug 2023 17:02:06 +0700 Subject: [PATCH 3/4] Also inject before final --- packages/vite/src/streamHelpers.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/vite/src/streamHelpers.ts b/packages/vite/src/streamHelpers.ts index 848dc877cef1..cb8fe4e8b5d0 100644 --- a/packages/vite/src/streamHelpers.ts +++ b/packages/vite/src/streamHelpers.ts @@ -117,6 +117,14 @@ function createServerInjectionStream({ next() }, final() { + // Before finishing, make sure we flush anything else that has been added to the queue + // Because of the implementation, its safe to call this multiple times (I think!) + // This is really for the data fetching usecase, where the promise is resolved after is closed + const elementsAtTheEnd = renderToString( + React.createElement(ServerInjectedHtml) + ) + + outputStream.write(elementsAtTheEnd) onFinal() }, }) From 1737b0227a7b72b05bb81ae8faf4a622983ece36 Mon Sep 17 00:00:00 2001 From: Daniel Choudhury Date: Fri, 4 Aug 2023 21:02:38 +0700 Subject: [PATCH 4/4] Try injection state isolation --- packages/vite/src/streamHelpers.ts | 28 +++++++-- packages/web/src/components/ServerInject.tsx | 60 ++++++++++---------- 2 files changed, 55 insertions(+), 33 deletions(-) diff --git a/packages/vite/src/streamHelpers.ts b/packages/vite/src/streamHelpers.ts index cb8fe4e8b5d0..47ec04af8879 100644 --- a/packages/vite/src/streamHelpers.ts +++ b/packages/vite/src/streamHelpers.ts @@ -11,6 +11,8 @@ import type { TagDescriptor } from '@redwoodjs/web' import { ServerHtmlProvider, ServerInjectedHtml, + createInjector, + RenderCallback, } from '@redwoodjs/web/dist/components/ServerInject' interface RenderToStreamArgs { @@ -50,17 +52,29 @@ export function reactRenderToStream({ meta: metaTags, }) + // This ensures an isolated state for each request + const { injectionState, injectToPage } = createInjector() + console.log(`👉 \n ~ file: streamHelpers.ts:57 ~ injectToPage:`, injectToPage) + console.log( + `👉 \n ~ file: streamHelpers.ts:57 ~ injectionState:`, + injectionState + ) + + // This is effectively a transformer stream const intermediateStream = createServerInjectionStream({ outputStream: res, onFinal: () => { res.end() }, + injectionState, }) const { pipe } = renderToPipeableStream( React.createElement( ServerHtmlProvider, - {}, + { + value: injectToPage, + }, ServerEntry({ url: currentPathName, css: FIXME_HardcodedIndexCss, @@ -83,9 +97,11 @@ export function reactRenderToStream({ function createServerInjectionStream({ outputStream, onFinal, + injectionState, }: { outputStream: Writable onFinal: () => void + injectionState: Set }) { return new Writable({ write(chunk, encoding, next) { @@ -97,7 +113,9 @@ function createServerInjectionStream({ const [beforeClosingHead, afterClosingHead] = split const elementsInjectedToHead = renderToString( - React.createElement(ServerInjectedHtml) + React.createElement(ServerInjectedHtml, { + injectionState, + }) ) const outputBuffer = Buffer.from( @@ -118,10 +136,12 @@ function createServerInjectionStream({ }, final() { // Before finishing, make sure we flush anything else that has been added to the queue - // Because of the implementation, its safe to call this multiple times (I think!) + // Because of the implementation in ServerRenderHtml, its safe to call this multiple times (I think!) // This is really for the data fetching usecase, where the promise is resolved after is closed const elementsAtTheEnd = renderToString( - React.createElement(ServerInjectedHtml) + React.createElement(ServerInjectedHtml, { + injectionState, + }) ) outputStream.write(elementsAtTheEnd) diff --git a/packages/web/src/components/ServerInject.tsx b/packages/web/src/components/ServerInject.tsx index e38eddba891a..93c011579deb 100644 --- a/packages/web/src/components/ServerInject.tsx +++ b/packages/web/src/components/ServerInject.tsx @@ -1,56 +1,58 @@ -import React, { - Fragment, - PropsWithChildren, - ReactNode, - useContext, -} from 'react' +import React, { Fragment, ReactNode, useContext } from 'react' /** * * Inspired by Next's useServerInsertedHTML, originally designed for CSS-in-JS * for now it seems the only way to inject html with streaming is to use a context * + * We use this for tags, and for apollo cache hydration + * * Until https://github.com/reactjs/rfcs/pull/219 makes it into react * */ -type RenderCallback = () => ReactNode - -const insertCallbacks: Set = new Set([]) +export type RenderCallback = () => ReactNode export const ServerHtmlContext = React.createContext< ((things: RenderCallback) => void) | null >(null) -const injectToHead = (renderCallback: RenderCallback) => { - insertCallbacks.add(renderCallback) -} +/** + * + * Use this factory, once per request. + * This is to ensure that injectionState is isolated to the request + * and not shared between requests. + */ +export const createInjector = () => { + const injectionState: Set = new Set([]) -// @MARK: I don't know why Next don't do this also? -// My understanding: put the inject function in a context so that -// ServerInjectedHTML component rerenders, even though the state isn't inside the context -export const ServerHtmlProvider: React.FC = ({ - children, -}) => { - return ( - - {children} - - ) + const injectToPage = (renderCallback: RenderCallback) => { + injectionState.add(renderCallback) + } + + return { injectToPage, injectionState } } -export const ServerInjectedHtml = () => { +// @NOTE do not instatiate the provider value here, so that we can ensure +// context isolation. This is done in streamHelpers currently, +// using the createInjector factory, once per request +export const ServerHtmlProvider = ServerHtmlContext.Provider + +export const ServerInjectedHtml = ({ + injectionState, +}: { + injectionState: Set +}) => { const serverInsertedHtml = [] - for (const callback of insertCallbacks) { + for (const callback of injectionState) { serverInsertedHtml.push(callback()) - // Remove it from the set so its not called again - insertCallbacks.delete(callback) + // Remove it from the set so its not rendered again + injectionState.delete(callback) } - // @MARK: using i as key here might be problematic, no? return serverInsertedHtml.map((html, i) => { - return {html} + return {html} }) }