-
Notifications
You must be signed in to change notification settings - Fork 212
Route serialization to support Shopper SEO URL mapping integration #2300
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
Changes from 18 commits
f178dec
7b8b80b
4f30443
55c4415
5da9d8e
4d13fe3
8a54875
4604d76
e6542fc
7a1f51f
ddb88d2
138f44b
3bfa646
2054078
a547b24
63a9d1f
d90c79c
c67fefa
af9f3df
b002c86
bd74cf8
b84c8c3
3a9f90d
4a3a6ce
6d92af9
9f6c055
6d5fb39
0115d21
fa85193
040e3f1
86a78ed
0a7e855
9acff92
c5d924b
2206dd5
392387a
0c1bd41
08e72bd
0dbbb34
8324897
5586726
645a91f
d0ffc37
f97aaed
42879f5
b71c0b9
bb692bc
0d2ed3b
35a453e
272639f
2067892
0e44b84
1579626
806fa87
85f9516
11b5af3
6402e62
e8bba80
af93217
579bdf3
18adf16
d9b32e2
4ad2535
965a34a
6baf649
5dbe489
26da1ee
f1eab7c
0a1626c
c622823
92a1c9a
f615878
38e352f
4119cda
6677213
0058e80
fa4540a
4af8642
7fdd261
bcdcea3
7b48662
62e0060
347c486
de121c3
2152142
ab23cd6
5236d90
131f670
6e257cd
edcae1d
04ee8fb
c6be840
c8e8035
88419bf
e9c00dd
1de2122
38425c2
2bcbbc2
7ba4d99
7c731f2
a2096c3
18a566f
0e0edcb
422bd78
7ed9fbd
e08d7ce
bab7884
c594557
794b0a3
0cd67cc
54ec28a
6d7c6a9
12b071b
5f5de44
ad8933a
f65cf58
876b4c2
ad6d337
f05a70a
16f461b
052c99f
6d32d9d
6e3a71d
8b17606
d1f42db
3cc8230
23f3f26
673728e
d904807
ca31a1e
4ef75d1
46ccd7b
cc16ffc
a9a3bed
c658d3f
572e689
03dc975
152aa8a
6ed7d9f
b5cd201
d5d983e
3c48a34
73d4793
14325a9
d5e2de7
caa865b
a45ea36
9fbd8e7
7dc6873
713d4e0
fd025e6
5cf6e06
07e29dd
533020b
840161e
1504b0a
26a60d6
b43928d
585669d
d0299fb
73f09f1
66fc821
5bcec25
3d5febc
a49f564
a200b71
59c8eb7
e6405b4
02ef399
80a9fb0
24b2195
ba61551
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,7 @@ import {RouteProps} from 'react-router-dom' | |
| import {ApplicationExtension as ApplicationExtensionBase} from '../../shared/classes/application-extension-base' | ||
|
|
||
| // Types | ||
| import {ApplicationExtensionConfig} from '../../types' | ||
| import {ApplicationExtensionConfig, Modules, SerializedRoute} from '../../types' | ||
|
|
||
| export type ReactApplicationExtensionConfig = ApplicationExtensionConfig | ||
|
|
||
|
|
@@ -27,6 +27,10 @@ export type ReactApplicationExtensionConfig = ApplicationExtensionConfig | |
| export class ApplicationExtension< | ||
| Config extends ReactApplicationExtensionConfig | ||
| > extends ApplicationExtensionBase<Config> { | ||
| constructor(config: Config) { | ||
| super(config) | ||
| this.extendRoutes = this.extendRoutes.bind(this) | ||
| } | ||
| /** | ||
| * Called during the rendering of the base application on the server and the client. | ||
| * It is predominantly used to enhance the "base" application by wrapping it with React providers. | ||
|
|
@@ -50,8 +54,12 @@ export class ApplicationExtension< | |
| * @param routes - The list application routes currently loaded. | ||
| * @returns routes - The modified application routes. | ||
| */ | ||
| public extendRoutes(routes: RouteProps[]): RouteProps[] { | ||
| return routes | ||
| public extendRoutes(routes: RouteProps[]): Promise<RouteProps[]> { | ||
| return Promise.resolve(routes) | ||
| } | ||
|
|
||
| public getRoutes(): Promise<RouteProps[]> { | ||
| return Promise.resolve([]) | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -65,4 +73,69 @@ export class ApplicationExtension< | |
| public beforeRouteMatch(routes: RouteProps[]): RouteProps[] { | ||
| return routes | ||
| } | ||
|
|
||
| public async serialize(): Promise<SerializedRoute[]> { | ||
|
||
| const routes = await this.getRoutes() | ||
| return routes.map((route) => { | ||
| if (!route.component?.displayName) { | ||
|
Collaborator
Author
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. @bendvc I noticed that in However I noticed that Would I need to make any changes to the
Contributor
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 think thats a good idea. Currently there really isn't anything stopping extensions from having duplicate named pages. If you have time, we'll want to create a ticket for addressing that. No need to worry about that now, but we should do that at some point. |
||
| throw new Error(`Component for route with path "${route.path}" is missing a displayName`) | ||
| } | ||
| return { | ||
| path: route.path, | ||
| componentName: route.component?.displayName, | ||
| // TODO: do we need to serialize props? | ||
| //componentProps: route.props | ||
|
||
| } | ||
| }) | ||
| } | ||
|
|
||
| public deserialize(serializedRoutes: SerializedRoute[], componentMap: Modules): any[] { | ||
|
||
| return serializedRoutes.map( | ||
| ({path, componentName}) => { | ||
| let component = componentMap[componentName] | ||
| if (!component) { | ||
| throw new Error(`${componentName} component could not be deserialized for route with path: ${path}`) | ||
| } | ||
|
|
||
| // if (componentProps) { | ||
| // const Component = component | ||
| // component = () => <Component {...componentProps} /> | ||
| // } | ||
| return { | ||
| path, | ||
| exact: true, | ||
| component | ||
| } | ||
| } | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Protected method to get the component map. This method should be called | ||
| * by subclasses to provide the correct path for importing modules. | ||
| * | ||
| * @protected | ||
| * @param path - The path to the modules directory that contains the page components. | ||
| * @returns A promise that resolves to the component map. | ||
| */ | ||
| protected async generateComponentMapFromModules(modules: Modules): Promise<Modules> { | ||
| const componentMap = Object.keys(modules).reduce((acc: Modules, key: string) => { | ||
| const module = modules[key] | ||
| // TODO: will displayName always exist or do we need error handling? | ||
| acc[module.displayName] = module | ||
| return acc | ||
| }, {}) | ||
| return componentMap | ||
| } | ||
|
||
|
|
||
|
|
||
| // TODO: should we make an abstract class for getComponentMap to enforce | ||
| // subclasses to implement it? | ||
| /** | ||
| * Default method to get the component map using the default path './pages'. | ||
| * Subclasses can override this method if they need to use a different path. | ||
| * | ||
| * @returns A promise that resolves to the component map. | ||
| */ | ||
| // public abstract getComponentMap(): Promise<Modules> | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,7 +34,7 @@ import Document from '../universal/components/_document' | |
| import Throw404 from '../universal/components/throw-404' | ||
| import {getAppConfig} from '../universal/compatibility' | ||
| import Switch from '../universal/components/switch' | ||
| import {getRoutes, routeComponent} from '../universal/components/route-component' | ||
| import {getAllRoutes, routeComponent} from '../universal/components/route-component' | ||
| import * as errors from '../universal/errors' | ||
| import logger from '../../utils/logger-instance' | ||
| import PerformanceTimer, {PERFORMANCE_MARKS} from '../../utils/performance' | ||
|
|
@@ -139,7 +139,7 @@ export const render = async (req, res, next) => { | |
| locals: res.locals | ||
| }) | ||
|
|
||
| let routes = getRoutes(res.locals) | ||
| let routes = await getAllRoutes(res.locals) | ||
|
|
||
| const [pathname] = req.originalUrl.split('?') | ||
|
|
||
|
|
@@ -150,6 +150,16 @@ export const render = async (req, res, next) => { | |
| }) | ||
| } | ||
|
|
||
| // TODO: How do we create a new field in config.app called serializedExtensions to store the routes? | ||
| config.app.routes = Object.fromEntries( | ||
| await Promise.all( | ||
| applicationExtensions.map(async (extension) => { | ||
| const serializedData = await extension.serialize() | ||
| return [extension.constructor.name, serializedData] | ||
| }) | ||
| ) | ||
| ) | ||
|
||
|
|
||
| // Step 1 - Find the match. | ||
|
|
||
| // Call `beforeRouteMatch` application extension hook. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,7 @@ import hoistNonReactStatic from 'hoist-non-react-statics' | |
| import {AppErrorContext} from '../../components/app-error-boundary' | ||
| import Throw404 from '../../components/throw-404' | ||
| import {getAppConfig} from '../../compatibility' | ||
| import routes from '../../routes' | ||
| import appRoutes from '../../routes' | ||
| import {pages as pageEvents} from '../../events' | ||
| import {withLegacyGetProps} from '../../components/with-legacy-get-props' | ||
| import Refresh from '../refresh' | ||
|
|
@@ -396,30 +396,41 @@ export const routeComponent = (Wrapped, isPage, locals) => { | |
| } | ||
|
|
||
| /** | ||
| * TODO: update this comment | ||
| * Wrap all the components found in the application's route config with the | ||
| * route-component HOC so that they all support `getProps` methods server-side | ||
| * and client-side in the same way. | ||
| * | ||
| * @private | ||
| */ | ||
| export const getRoutes = (locals = {}) => { | ||
| let _routes = routes | ||
| export const getAllRoutes = async (locals = {}) => { | ||
| const {applicationExtensions = []} = locals | ||
| if (typeof routes === 'function') { | ||
| _routes = routes() | ||
| } | ||
|
|
||
| // Call the `extendRoutes` function for all the Application Extensions. | ||
| applicationExtensions.forEach((applicationExtension) => { | ||
| _routes = applicationExtension.extendRoutes(_routes) | ||
| }) | ||
| let extensionRoutes = [] | ||
| if (isServerSide){ | ||
| extensionRoutes = ( | ||
| await Promise.all(applicationExtensions.map((extension) => extension.getRoutes())) | ||
| ).flat() | ||
| } else { | ||
| // Deserialize the routes from the server | ||
| const serializedRoutes = window.__CONFIG__.app.routes | ||
| for (const applicationExtension of applicationExtensions) { | ||
| const extensionName = applicationExtension.constructor.name | ||
| const componentMap = await applicationExtension.getComponentMap() | ||
| const deserializedRoutes = applicationExtension.deserialize(serializedRoutes[extensionName], componentMap) | ||
| extensionRoutes = [...extensionRoutes, ...deserializedRoutes] | ||
| } | ||
| } | ||
|
||
|
|
||
| const allRoutes = [ | ||
| // NOTE: this route needs to be above _routes, in case _routes has a fallback route of `path: '*'` | ||
| {path: '/__pwa-kit/refresh', component: Refresh}, | ||
| ..._routes, | ||
| ...extensionRoutes, | ||
| ...(typeof appRoutes === 'function' ? appRoutes() : appRoutes), | ||
| {path: '*', component: Throw404} | ||
| ] | ||
| console.log('--- routes', allRoutes) | ||
|
|
||
| return allRoutes.map(({component, ...rest}) => { | ||
| return { | ||
| component: component ? routeComponent(component, true, locals) : component, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,12 +4,15 @@ | |
| * SPDX-License-Identifier: BSD-3-Clause | ||
| * For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause | ||
| */ | ||
| import React from 'react' | ||
| import React, {useContext, useState} from 'react' | ||
| import PropTypes from 'prop-types' | ||
| import {Switch as RouterSwitch, Route} from 'react-router-dom' | ||
| import AppErrorBoundary from '../app-error-boundary' | ||
| import {UIDReset, UIDFork} from 'react-uid' | ||
|
|
||
| const RoutesContext = React.createContext({}) | ||
| export const useRoutesContext = () => useContext(RoutesContext) | ||
|
||
|
|
||
| /** | ||
| * The Switch component packages up the bits of rendering that are shared between | ||
| * server and client-side. It's *mostly* a react-router Switch component, hence the | ||
|
|
@@ -21,24 +24,30 @@ import {UIDReset, UIDFork} from 'react-uid' | |
| */ | ||
| const Switch = (props) => { | ||
| const {error, appState, routes, App} = props | ||
| const [_routes, setRoutes] = useState(routes) | ||
| return ( | ||
| <UIDReset> | ||
| <AppErrorBoundary error={error}> | ||
| {!error && ( | ||
| <App preloadedProps={appState.appProps}> | ||
| <RouterSwitch> | ||
| {routes.map((route, i) => { | ||
| const {component: Component, ...routeProps} = route | ||
| return ( | ||
| <Route key={i} {...routeProps}> | ||
| <UIDFork> | ||
| <Component preloadedProps={appState.pageProps} /> | ||
| </UIDFork> | ||
| </Route> | ||
| ) | ||
| })} | ||
| </RouterSwitch> | ||
| </App> | ||
| <RoutesContext.Provider value={{ | ||
| routes: _routes, | ||
| setRoutes | ||
| }}> | ||
| <App preloadedProps={appState.appProps}> | ||
| <RouterSwitch> | ||
| {_routes.map((route, i) => { | ||
| const {component: Component, ...routeProps} = route | ||
| return ( | ||
| <Route key={i} {...routeProps}> | ||
| <UIDFork> | ||
| <Component preloadedProps={appState.pageProps} /> | ||
| </UIDFork> | ||
| </Route> | ||
| ) | ||
| })} | ||
| </RouterSwitch> | ||
| </App> | ||
| </RoutesContext.Provider> | ||
| )} | ||
| </AppErrorBoundary> | ||
| </UIDReset> | ||
|
|
||
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.
extension classes will need to implement the
getComponentMapfunction as putting this logic inApplicationExtension.getComponentMapgives errors when attempting to import from./pagessince this folder only exists in extensionsThere 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.
Can you remind me again why this is async?
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.
Good point this does not have to be async!