feat: expose `getRoutes` to share routes with private env.config.js files #1143

adamstankiewicz · 2024-08-06T21:32:32Z

Related PRs:

https://github.com/edx/edx-internal/pull/11384

CHANGELOG:

Refactors route data structure from <Route> JSX elements to an array of objects, such that the router tree may be exposed by a utility function getRoutes that may be shared between the MFE source code and env.config.js(x).
Migrates route-related and some query related code paths to TypeScript, ensuring proper typing in all route loaders, route definitions, some React Query related stuff, etc.

Given the output from getRoutes, the flattened route paths result is as follows:

[
    "/invite/:enterpriseCustomerInviteKey",
    "/:enterpriseSlug?",
    "/:enterpriseSlug?/search/:pathwayUUID?",
    "/:enterpriseSlug?/academies/:academyUUID",
    "/:enterpriseSlug?/pathway/:pathwayUUID/progress",
    "/:enterpriseSlug?/program/:programUUID",
    "/:enterpriseSlug?/program/:programUUID/progress",
    "/:enterpriseSlug?/skills-quiz",
    "/:enterpriseSlug?/:courseType?/course/:courseKey",
    "/:enterpriseSlug?/:courseType?/course/:courseKey/enroll/:courseRunKey",
    "/:enterpriseSlug?/:courseType?/course/:courseKey/enroll/:courseRunKey/complete",
    "/:enterpriseSlug?/licenses/:activationKey/activate",
    "/:enterpriseSlug?/videos/:videoUUID",
    "/:enterpriseSlug?/*",
    "/*"
]

TODO: Tests are WIP.

For all changes

Ensure adequate tests are in place (or reviewed existing tests cover changes)
Ensure English strings are marked for translation. See documentation for more details.

Only if submitting a visual change

Ensure to attach screenshots
Ensure to have UX team confirm screenshots

adamstankiewicz · 2024-08-06T21:44:20Z

package.json

    "@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",


[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.

[nit] does this still need to be either pinned/removed?

adamstankiewicz · 2024-08-06T21:44:34Z

package.json

    "@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",


[inform] should be pinned

adamstankiewicz · 2024-08-06T21:45:02Z

package.json

+    "react-router": "6.18.0",
+    "react-router-dom": "6.18.0",


[context] resolves a bug with matchPath and its handling of optional route parameters (e.g., /:enterpriseSlug?)

adamstankiewicz · 2024-08-06T21:45:38Z

src/components/app/Layout.jsx

@@ -44,7 +44,7 @@ const Layout = () => {
      <main id="content" className="fill-vertical-space">
        <Outlet />
      </main>
-      <SiteFooter />
+      <FooterSlot />


[inform] Misc. improvement to support a plugin-enabled footer; basically a no-op though.

adamstankiewicz · 2024-08-06T21:46:29Z

src/components/enterprise-page/EnterprisePage.jsx

+      // Set custom attributes for logging service
+      const loggingService = getLoggingService();
+      loggingService.setCustomAttribute('enterprise_customer_uuid', enterpriseCustomer.uuid);


[inform] Ensures enterprise_customer_uuid is tagged for Datadog RUM events (e.g., to facet/query session replays by specific enterprise customers).

adamstankiewicz · 2024-08-06T21:47:47Z

src/routes.jsx

+function getRouteLoader(routeLoaderFn, queryClient) {
+  if (!queryClient) {
+    return null;
+  }
+  return routeLoaderFn(queryClient);
+}


[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.

adamstankiewicz · 2024-08-06T21:49:02Z

src/routes.jsx

+      ),
+    },
+  ];
+  if (hasNotFoundRoutes) {


[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.

adamstankiewicz · 2024-08-06T21:49:21Z

src/routes.jsx

+    ...getOtherRoutes(),
+    ...enterpriseSlugRoutes,
+  ];
+  if (hasNotFoundRoutes) {


[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.

adamstankiewicz · 2024-08-06T21:49:46Z

src/routes.jsx

+  };
+}
+
+export function extractPaths(routes, basePath = '/') {


[inform] needs docstring / types

adamstankiewicz · 2024-08-06T21:49:56Z

src/routes.jsx

+  return paths;
+}
+
+export function replaceRouteParamsInPath(viewPath, routePaths) {


[inform] needs docstring / types

adamstankiewicz · 2024-08-06T21:50:20Z

src/routes.jsx

+      if (!value) {
+        return;
+      }
+      viewPathCopy = viewPathCopy.replaceAll(value, '?');


[inform] the most important part, finally 😜

brobro10000 · 2024-08-07T12:58:57Z

src/routes.jsx

+    if (route.children?.length > 0) {
+      paths = [...paths, ...extractPaths(route.children, `${currentPath}/`)];
+    }


[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}/`)]; } });

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

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; }

adamstankiewicz · 2024-08-08T13:15:44Z

tsconfig.json

+    "jsx": "react-jsx",
+    "lib": ["ES2021"],


[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 · 2024-08-08T13:32:56Z

Codecov Report

Attention: Patch coverage is 96.39640% with 12 lines in your changes missing coverage. Please review.

Project coverage is 88.05%. Comparing base (42c06b0) to head (066857f).

Files	Patch %	Lines
src/components/utils/search.js	60.00%	8 Missing ⚠️
src/components/app/data/queries/queryKeyFactory.js	93.10%	2 Missing ⚠️
...c/components/skills-quiz/SkillsContextProvider.jsx	0.00%	2 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

brobro10000

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 👍🏽

brobro10000 · 2024-08-12T12:07:43Z

package.json

    "@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",


[nit] does this still need to be either pinned/removed?

brobro10000 · 2024-08-12T12:30:02Z

src/components/app/routes/loaders/rootLoader.ts

@@ -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,


[nit] possible test data left in?

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.

Ohh yeah I am just noticing its not wrapped in a set of quotation marks denoting as a string 😅 .

brobro10000 · 2024-08-12T12:30:30Z

src/components/app/routes/loaders/tests/rootLoader.test.jsx

+    // {
+    //   enterpriseSlug: mockEnterpriseCustomerTwo.slug,
+    //   enterpriseCustomer: mockEnterpriseCustomerTwo,
+    //   activeEnterpriseCustomer: mockEnterpriseCustomer,
+    //   allLinkedEnterpriseCustomerUsers: [
+    //     { enterpriseCustomer: mockEnterpriseCustomer },
+    //     { enterpriseCustomer: mockEnterpriseCustomerTwo },
+    //   ],
+    //   isStaffUser: false,
+    //   shouldRedirectToSearch: false,
+    // },


[nit] some commented out code.

D'oh, great catch!

brobro10000 · 2024-08-12T12:32:20Z

src/components/course/data/courseLoader.ts

@@ -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 }));


[nit] some remaining test data

Unlike the other instance, this one can be removed, though. The CourseRouteParams coerces the enterpriseSlug to only be string already.

brobro10000 · 2024-08-12T12:34:26Z

src/components/course/routes/tests/externalCourseEnrollmentLoader.test.jsx

+// 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(),
+// }));


[nit] some commented out code

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.

brobro10000 · 2024-08-12T12:40:05Z

src/routes.tsx

+/**
+ * Returns the routes nested under the enterprise slug prefix.
+ */
+function getEnterpriseSlugRoutes(queryClient?: Types.QueryClient) {


[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)

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.

Thank you for this explanation! Leave unresolved for knowledge sharing purposes IMO.

feat: expose getRoutes API to share with private env.config.js files

8267e49

adamstankiewicz commented Aug 6, 2024

View reviewed changes

src/routes.jsx Outdated

};

}

export function extractPaths(routes, basePath = '/') {

Copy link

Member Author

adamstankiewicz Aug 6, 2024

Choose a reason for hiding this comment

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

[inform] needs docstring / types

adamstankiewicz commented Aug 6, 2024

View reviewed changes

adamstankiewicz changed the title ~~feat: expose getRoutes API to share with private env.config.js files~~ feat: expose getRoutes to share routes with private env.config.js files Aug 6, 2024

brobro10000 reviewed Aug 7, 2024

View reviewed changes

adamstankiewicz added 6 commits August 7, 2024 16:41

refactor: migrate route code paths to typescript

5a85959

chore: fix a type

7b44068

chore: wip

c69be6b

chore: updates

1a6b7f1

Merge branch 'master' into ags/ent-9274

041480f

chore: use react-jsx in tsconfig.json

9c24f6a

adamstankiewicz commented Aug 8, 2024

View reviewed changes

chore: lint

710603c

adamstankiewicz added 3 commits August 8, 2024 12:32

chore: upgrade @edx/typescript-config

084b803

chore: updates

e0c3b2e

chore: test fix

4828ee2

brobro10000 approved these changes Aug 12, 2024

View reviewed changes

adamstankiewicz marked this pull request as ready for review August 12, 2024 13:39

adamstankiewicz added 2 commits August 12, 2024 10:10

chore: pr feedback

14576b1

chore: commit missing type

307d6ff

chore: fix comments

066857f

adamstankiewicz merged commit 10362ba into master Aug 12, 2024
7 checks passed

adamstankiewicz deleted the ags/ent-9274 branch August 12, 2024 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expose `getRoutes` to share routes with private env.config.js files #1143

feat: expose `getRoutes` to share routes with private env.config.js files #1143

adamstankiewicz commented Aug 6, 2024 •

edited

Loading

adamstankiewicz Aug 6, 2024 •

edited

Loading

brobro10000 Aug 12, 2024

adamstankiewicz Aug 6, 2024

adamstankiewicz Aug 6, 2024 •

edited

Loading

adamstankiewicz Aug 6, 2024

adamstankiewicz Aug 6, 2024

adamstankiewicz Aug 6, 2024

adamstankiewicz Aug 6, 2024

adamstankiewicz Aug 6, 2024

adamstankiewicz Aug 6, 2024

adamstankiewicz Aug 6, 2024

adamstankiewicz Aug 6, 2024

brobro10000 Aug 7, 2024 •

edited

Loading

iloveagent57 Aug 7, 2024

adamstankiewicz Aug 8, 2024 •

edited

Loading

adamstankiewicz Aug 8, 2024

codecov bot commented Aug 8, 2024 •

edited

Loading

brobro10000 left a comment

brobro10000 Aug 12, 2024

brobro10000 Aug 12, 2024

adamstankiewicz Aug 12, 2024

brobro10000 Aug 12, 2024

brobro10000 Aug 12, 2024

adamstankiewicz Aug 12, 2024

brobro10000 Aug 12, 2024

adamstankiewicz Aug 12, 2024

brobro10000 Aug 12, 2024

adamstankiewicz Aug 12, 2024

brobro10000 Aug 12, 2024

adamstankiewicz Aug 12, 2024 •

edited

Loading

brobro10000 Aug 12, 2024

feat: expose getRoutes to share routes with private env.config.js files #1143

feat: expose getRoutes to share routes with private env.config.js files #1143

Conversation

adamstankiewicz commented Aug 6, 2024 • edited Loading

For all changes

Only if submitting a visual change

adamstankiewicz Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamstankiewicz Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brobro10000 Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamstankiewicz Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 8, 2024 • edited Loading

Codecov Report

brobro10000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamstankiewicz Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: expose `getRoutes` to share routes with private env.config.js files #1143

feat: expose `getRoutes` to share routes with private env.config.js files #1143

adamstankiewicz commented Aug 6, 2024 •

edited

Loading

adamstankiewicz Aug 6, 2024 •

edited

Loading

adamstankiewicz Aug 6, 2024 •

edited

Loading

brobro10000 Aug 7, 2024 •

edited

Loading

adamstankiewicz Aug 8, 2024 •

edited

Loading

codecov bot commented Aug 8, 2024 •

edited

Loading

adamstankiewicz Aug 12, 2024 •

edited

Loading