-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: expose getRoutes
to share routes with private env.config.js files
#1143
Conversation
package.json
Outdated
"@edx/frontend-enterprise-catalog-search": "10.5.0", | ||
"@edx/frontend-enterprise-hotjar": "6.0.0", | ||
"@edx/frontend-enterprise-logistration": "8.0.0", | ||
"@edx/frontend-enterprise-utils": "8.0.0", | ||
"@edx/frontend-logging": "^4.3.0", |
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.
[inform] this needs to be removed from this file. it a private NPM package installed only by 2U's GoCD build+deploy MFE pipeline via env.config
.
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.
[nit] does this still need to be either pinned/removed?
package.json
Outdated
"@edx/frontend-platform": "7.1.0", | ||
"@edx/openedx-atlas": "0.6.0", | ||
"@edx/reactifex": "2.2.0", | ||
"@loadable/component": "5.16.3", | ||
"@lukemorales/query-key-factory": "1.3.4", | ||
"@openedx/frontend-slot-footer": "^1.0.3", |
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.
[inform] should be pinned
"react-router": "6.18.0", | ||
"react-router-dom": "6.18.0", |
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.
[context] resolves a bug with matchPath
and its handling of optional route parameters (e.g., /:enterpriseSlug?
)
@@ -44,7 +44,7 @@ const Layout = () => { | |||
<main id="content" className="fill-vertical-space"> | |||
<Outlet /> | |||
</main> | |||
<SiteFooter /> | |||
<FooterSlot /> |
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.
[inform] Misc. improvement to support a plugin-enabled footer; basically a no-op though.
// Set custom attributes for logging service | ||
const loggingService = getLoggingService(); | ||
loggingService.setCustomAttribute('enterprise_customer_uuid', enterpriseCustomer.uuid); |
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.
[inform] Ensures enterprise_customer_uuid
is tagged for Datadog RUM events (e.g., to facet/query session replays by specific enterprise customers).
src/routes.jsx
Outdated
function getRouteLoader(routeLoaderFn, queryClient) { | ||
if (!queryClient) { | ||
return null; | ||
} | ||
return routeLoaderFn(queryClient); | ||
} |
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.
[inform] When getRoutes
is used from env.config.js
, there is not yet access to a queryClient
. Instead, I've made getRoutes
conditional such that route loaders that depend on the queryClient
are only included in the route definitions if a queryClient
is present.
src/routes.jsx
Outdated
), | ||
}, | ||
]; | ||
if (hasNotFoundRoutes) { |
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.
[inform] need to sanity check if the two conditional NotFoundPage
routes (based on hasNotFoundRoutes
) are still needed now that the approach has changed slightly.
Hunch is these two routes can safely always be included now, simplifying this logic slightly.
src/routes.jsx
Outdated
...getOtherRoutes(), | ||
...enterpriseSlugRoutes, | ||
]; | ||
if (hasNotFoundRoutes) { |
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.
[inform] need to sanity check if the two conditional NotFoundPage
routes (based on hasNotFoundRoutes
) are still needed now that the approach has changed slightly.
Hunch is these two routes can safely always be included now, simplifying this logic slightly.
src/routes.jsx
Outdated
}; | ||
} | ||
|
||
export function extractPaths(routes, basePath = '/') { |
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.
[inform] needs docstring / types
src/routes.jsx
Outdated
return paths; | ||
} | ||
|
||
export function replaceRouteParamsInPath(viewPath, routePaths) { |
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.
[inform] needs docstring / types
src/routes.jsx
Outdated
if (!value) { | ||
return; | ||
} | ||
viewPathCopy = viewPathCopy.replaceAll(value, '?'); |
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.
[inform] the most important part, finally 😜
getRoutes
to share routes with private env.config.js files
src/routes.jsx
Outdated
if (route.children?.length > 0) { | ||
paths = [...paths, ...extractPaths(route.children, `${currentPath}/`)]; | ||
} |
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.
[nit/question] Since its recursive, should there be a base case before entering the possible recursion that checks explicitly for !route.children?.length
and return
routes.forEach((route) => {
let currentPath = basePath;
// Append the current route's path to the base path
if (route.path) {
currentPath += route.path;
}
if (!route.index) {
paths.push(currentPath);
}
if(!route.children?.length) {
return
}
// Recursively handle nested routes (children)
if (route.children?.length > 0) {
paths = [...paths, ...extractPaths(route.children, `${currentPath}/`)];
}
});
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.
I had a similar question - I think you'd want to terminate and return an empty list when there aren't any more children of a route, maybe? Right now, when there aren't children, we're eventually, after the for-loop exhausts all the routes, going to return all of paths
as the recursive response at the top of the recursion stack. The recursion inside the for-loop is throwing me off, so I could easily be wrong. :p
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.
Another way to think of this logic (wrt the interaction between the recursion and the .forEach
) might be that the recursive logic is on a individual, per-route basis, but shares/extends/concatenates to the same array of flattened paths we've already come across.
The base case exists but is implicit, with the existing conditional on whether there are route.children
defined. Without route.children
, it pushes the current route's path to the flattened list and moves onto the next route in the top-level list, if any (essentially an early return). With children
, it traverses the nested routes for their paths
, continuing to push onto the same flattened array from the first iteration.
Example input:
const routes = [
{
path: '/:enterpriseSlug?',
element: <Outlet />, // Outlet renders child route(s)
loader: [() => {}], // exemplary
children: [
{
index: true,
element: <Dashboard />,
},
{
path: 'search',
element: <Search />,
},
{
path: ':courseType?/course/:courseKey',
element: <Outlet />,
children: [
{
index: true,
element: <CourseAbout />,
},
{
path: 'enroll/:courseRunKey',
element: <CourseEnroll />,
},
],
},
// ...
]
}
];
Example output:
[
'/:enterpriseSlug?',
'/:enterpriseSlug?/search',
'/:enterpriseSlug?/:courseType?/course/:courseKey',
'/:enterpriseSlug?/:courseType?/course/:courseKey/enroll/:courseRunKey',
// ...
]
See the PR description for full list of flattened route paths contained by the application, generated by this recursive logic.
Right now, when there aren't children, we're eventually, after the for-loop exhausts all the routes, going to return all of paths as the recursive response at the top of the recursion stack.
Correct, if a route has no children, it only adds its own path to the known paths
(if any). With children, it collects all the nested routes into paths
, too. Once the .forEach
exhausts all the routes, it returns all the paths
that were collected as a flattened array.
[inform/aside] I plan on simplifying it slightly as the following:
export function flattenRoutePaths(routes: Types.RouteObject[], basePath = '/') {
let paths: string[] = [];
routes.forEach((route) => {
// Construct the full path by combining basePath with route.path
const fullPath = `${basePath}${(route.path || '')}`;
// Add the full path to the result if the route has a path
if (route.path) {
paths.push(fullPath);
}
// Recursively process the route's children (if any)
if (Array.isArray(route.children)) {
paths = paths.concat(flattenRoutePaths(route.children, `${fullPath}/`));
}
});
return paths;
}
tsconfig.json
Outdated
"jsx": "react-jsx", | ||
"lib": ["ES2021"], |
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.
[inform] Hoping to contribute these upstream to @edx/typescript-config
, as they are generally applicable to all MFEs, not just this one. See openedx/typescript-config#12 for more details.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1143 +/- ##
==========================================
+ Coverage 87.12% 88.05% +0.92%
==========================================
Files 392 393 +1
Lines 8172 8268 +96
Branches 1984 1984
==========================================
+ Hits 7120 7280 +160
+ Misses 1002 946 -56
+ Partials 50 42 -8 ☔ View full report in Codecov by Sentry. |
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.
Couple of nits that needs to be addressed over (what looks like) some leftover test data and commented out code bits.
One non-blocking question about the queryClient being set to optional as an argument.
Approving to unblock 👍🏽
package.json
Outdated
"@edx/frontend-enterprise-catalog-search": "10.5.0", | ||
"@edx/frontend-enterprise-hotjar": "6.0.0", | ||
"@edx/frontend-enterprise-logistration": "8.0.0", | ||
"@edx/frontend-enterprise-utils": "8.0.0", | ||
"@edx/frontend-logging": "^4.3.0", |
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.
[nit] does this still need to be either pinned/removed?
@@ -73,7 +73,7 @@ export default function makeRootLoader(queryClient) { | |||
|
|||
// Redirect user to search page, for first-time users with no assignments. | |||
redirectToSearchPageForNewUser({ | |||
enterpriseSlug, | |||
enterpriseSlug: enterpriseSlug as string, |
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.
[nit] possible test data left in?
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.
Now that this is TypeScript, it knows enterpriseSlug
is either undefined or a string based on the params
object type. The enterpriseSlug as string
is coercing the value as a string given the redirectToSearchPageForNewUser
does not accept enterpriseSlug
as undefined, thus throwing a TypeScript error. In this part of the code, enterpriseSlug
is known to only be a string, so this is telling TypeScript that.
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.
Ohh yeah I am just noticing its not wrapped in a set of quotation marks denoting as a string 😅 .
// { | ||
// enterpriseSlug: mockEnterpriseCustomerTwo.slug, | ||
// enterpriseCustomer: mockEnterpriseCustomerTwo, | ||
// activeEnterpriseCustomer: mockEnterpriseCustomer, | ||
// allLinkedEnterpriseCustomerUsers: [ | ||
// { enterpriseCustomer: mockEnterpriseCustomer }, | ||
// { enterpriseCustomer: mockEnterpriseCustomerTwo }, | ||
// ], | ||
// isStaffUser: false, | ||
// shouldRedirectToSearch: false, | ||
// }, |
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.
[nit] some commented out code.
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.
D'oh, great catch!
@@ -115,7 +125,7 @@ export default function makeCourseLoader(queryClient) { | |||
// If learner is an assignment-only learner and is not assigned to the currently | |||
// viewed course, redirect to the Dashboard page route. | |||
if (isAssignmentOnlyLearner && !isCourseAssigned) { | |||
throw redirect(generatePath('/:enterpriseSlug', { enterpriseSlug })); | |||
throw redirect(generatePath('/:enterpriseSlug', { enterpriseSlug: enterpriseSlug as string })); |
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.
[nit] some remaining test data
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.
Unlike the other instance, this one can be removed, though. The CourseRouteParams
coerces the enterpriseSlug
to only be string already.
// jest.mock('@edx/frontend-platform/auth', () => ({ | ||
// ...jest.requireActual('@edx/frontend-platform/auth'), | ||
// configure: jest.fn(), | ||
// })); | ||
// jest.mock('@edx/frontend-platform/logging', () => ({ | ||
// ...jest.requireActual('@edx/frontend-platform/logging'), | ||
// configure: jest.fn(), | ||
// getLoggingService: jest.fn(), | ||
// })); |
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.
[nit] some commented out code
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 catch! I recall intentionally leaving this commented when I copied it from another route loader, as I had a hunch it wasn't needed (it wasn't). I will clean up the other loader tests that have these same Jest mocks unnecessarily.
/** | ||
* Returns the routes nested under the enterprise slug prefix. | ||
*/ | ||
function getEnterpriseSlugRoutes(queryClient?: Types.QueryClient) { |
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.
[question/curious] If the loaders are now gating for the query client should the query client be an optional argument? Are there any instances where we don't expect the query client to exist before the routes are loaded and that being an acceptable outcome (as to not gate our loaders with the following code snippet)?
if (!authenticatedUser || !queryClient) {
return null;
}
Should we just make the queryClient a non optional as an additional gating step for getEnterpriseSlugRoutes
? (This might just be a gap in my TS knowledge too)
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.
getRoutes
is called in two places: createAppRouter
(internal) and env.config.js
(external). Because env.config
is calling getRoutes
without passing a queryClient
(i.e., one does not exist yet), the getRoutes
and child functions like getEnterpriseSlugRoutes
need to conditionally handle when queryClient
is missing. The interaction with getRouteLoader
checks whether queryClient
is present and, if so, passed it to the route loader.
The shared route loader types themselves (i.e.,Types.MakeRouteLoaderFunction
) defines any route loader creator functions as conditionally accepting a queryClient
as not all route loaders in the app use a query client.
To be able to remove the if (!authenticatedUser || !queryClient) {
gating in a route loader, we could create a new loader-specific type that extends the base Types.MakeRouteLoaderFunction
type, telling TypeScript that this particular loader requires queryClient
.
// base makeRouteLoader type
export type MakeRouteLoaderFunction = (queryClient?: QueryClient) => RouteLoaderFunction;
// extended loader-specific makeRouteLoaderType that requires queryClient
type MakeRootLoaderFunction = (queryClient: Types.QueryClient) => Types.RouteLoaderFunction;
const makeRootLoader: MakeRootLoaderFunction = function makeRootLoader(queryClient) {
// no more check for `!queryClient` needed
}
Edit: I'm going to introduce a new common type:
export type MakeRouteLoaderFunction = (queryClient?: QueryClient) => RouteLoaderFunction;
export type MakeRouteLoaderFunctionWithQueryClient = (queryClient: QueryClient) => RouteLoaderFunction;
Any route loader that requires queryClient
will use MakeRouteLoaderFunctionWithQueryClient
; all others can use MakeRouteLoaderFunction
.
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.
Thank you for this explanation! Leave unresolved for knowledge sharing purposes IMO.
Ticket: ENT-9274
Related PRs:
CHANGELOG:
<Route>
JSX elements to an array of objects, such that the router tree may be exposed by a utility functiongetRoutes
that may be shared between the MFE source code andenv.config.js(x)
.Given the output from
getRoutes
, the flattened route paths result is as follows:TODO: Tests are WIP.
For all changes
Only if submitting a visual change