Navigation refactoring, handling extension page params (#1651)

* decentralizing page url-params management -- PoC / tsc 4.1 random fixes

Signed-off-by: Roman <ixrock@gmail.com>

* fixes, tweak example-extension for demo

Signed-off-by: Roman <ixrock@gmail.com>

* lint fixes, revert tests

Signed-off-by: Roman <ixrock@gmail.com>

* removed occasional changes related to typescript 4.1

Signed-off-by: Roman <ixrock@gmail.com>

* updated example with 2 menu-items targeting same page with different params

Signed-off-by: Roman <ixrock@gmail.com>

* fix: merge page url chunks with native URL()-api, simplified default page-params registration

Signed-off-by: Roman <ixrock@gmail.com>

* fix: make lint happy

Signed-off-by: Roman <ixrock@gmail.com>

* fix: unit-tests

Signed-off-by: Roman <ixrock@gmail.com>

* renaming by jim's request: UrlParam => PageParam (type), createUrlParam => createPageParam (helper)

Signed-off-by: Roman <ixrock@gmail.com>

* fix: reverting NamespaceStore public-api breaking changes

Signed-off-by: Roman <ixrock@gmail.com>

* lint fix

Signed-off-by: Roman <ixrock@gmail.com>

* fine-tuning

Signed-off-by: Roman <ixrock@gmail.com>

* yes, lint always unhappy

Signed-off-by: Roman <ixrock@gmail.com>

* fix build

Signed-off-by: Roman <ixrock@gmail.com>

* small fixes

Signed-off-by: Roman <ixrock@gmail.com>

* fix merge-conflicts

Signed-off-by: Roman <ixrock@gmail.com>

* removed `isSystem` page-param's init field exposed to extensions-api

Signed-off-by: Roman <ixrock@gmail.com>

src/extensions/cluster-feature.ts

@@ -41,7 +41,7 @@ export abstract class ClusterFeature {
   /**
    * to be implemented in the derived class, this method is typically called by Lens when a user has indicated that this feature is to be installed. The implementation
    * of this method should install kubernetes resources using the applyResources() method, or by directly accessing the kubernetes api (K8sApi)
-   * 
+   *
    * @param cluster the cluster that the feature is to be installed on
    */
   abstract async install(cluster: Cluster): Promise<void>;
@@ -49,7 +49,7 @@ export abstract class ClusterFeature {
   /**
    * to be implemented in the derived class, this method is typically called by Lens when a user has indicated that this feature is to be upgraded. The implementation
    * of this method should upgrade the kubernetes resources already installed, if relevant to the feature
-   * 
+   *
    * @param cluster the cluster that the feature is to be upgraded on
    */
   abstract async upgrade(cluster: Cluster): Promise<void>;
@@ -57,26 +57,26 @@ export abstract class ClusterFeature {
   /**
    * to be implemented in the derived class, this method is typically called by Lens when a user has indicated that this feature is to be uninstalled. The implementation
    * of this method should uninstall kubernetes resources using the kubernetes api (K8sApi)
-   * 
+   *
    * @param cluster the cluster that the feature is to be uninstalled from
    */
   abstract async uninstall(cluster: Cluster): Promise<void>;
 
   /**
    * to be implemented in the derived class, this method is called periodically by Lens to determine details about the feature's current status. The implementation
-   * of this method should provide the current status information. The currentVersion and latestVersion fields may be displayed by Lens in describing the feature. 
+   * of this method should provide the current status information. The currentVersion and latestVersion fields may be displayed by Lens in describing the feature.
    * The installed field should be set to true if the feature has been installed, otherwise false. Also, Lens relies on the canUpgrade field to determine if the feature
    * can be upgraded so the implementation should set the canUpgrade field according to specific rules for the feature, if relevant.
-   * 
+   *
    * @param cluster the cluster that the feature may be installed on
-   * 
+   *
    * @return a promise, resolved with the updated ClusterFeatureStatus
    */
   abstract async updateStatus(cluster: Cluster): Promise<ClusterFeatureStatus>;
 
   /**
    * this is a helper method that conveniently applies kubernetes resources to the cluster.
-   * 
+   *
    * @param cluster the cluster that the resources are to be applied to
    * @param resourceSpec as a string type this is a folder path that is searched for files specifying kubernetes resources. The files are read and if any of the resource
    * files are templated, the template parameters are filled using the templateContext field (See renderTemplate() method). Finally the resources are applied to the
@@ -101,9 +101,9 @@ export abstract class ClusterFeature {
   /**
    * this is a helper method that conveniently reads kubernetes resource files into a string array. It also fills templated resource files with the template parameter values
    * specified by the templateContext field. Templated files must end with the extension '.hb' and the template syntax must be compatible with handlebars.js
-   * 
+   *
    * @param folderPath this is a folder path that is searched for files defining kubernetes resources.
-   * 
+   *
    * @return an array of strings, each string being the contents of a resource file found in the folder path. This can be passed directly to applyResources()
    */
   protected renderTemplates(folderPath: string): string[] {

src/extensions/interfaces/registrations.ts

@@ -3,6 +3,6 @@ export type { ClusterFeatureRegistration, ClusterFeatureComponents } from "../re
 export type { KubeObjectDetailRegistration, KubeObjectDetailComponents } from "../registries/kube-object-detail-registry";
 export type { KubeObjectMenuRegistration, KubeObjectMenuComponents } from "../registries/kube-object-menu-registry";
 export type { KubeObjectStatusRegistration } from "../registries/kube-object-status-registry";
-export type { PageRegistration, PageComponents } from "../registries/page-registry";
-export type { PageMenuRegistration, PageMenuComponents } from "../registries/page-menu-registry";
+export type { PageRegistration, RegisteredPage, PageParams, PageComponentProps, PageComponents, PageTarget } from "../registries/page-registry";
+export type { PageMenuRegistration, ClusterPageMenuRegistration, PageMenuComponents } from "../registries/page-menu-registry";
 export type { StatusBarRegistration } from "../registries/status-bar-registry";

src/extensions/lens-renderer-extension.ts

@@ -1,14 +1,13 @@
-import type { AppPreferenceRegistration, ClusterFeatureRegistration, KubeObjectDetailRegistration, KubeObjectMenuRegistration, KubeObjectStatusRegistration, PageMenuRegistration, PageRegistration, StatusBarRegistration, } from "./registries";
+import type { AppPreferenceRegistration, ClusterFeatureRegistration, ClusterPageMenuRegistration, KubeObjectDetailRegistration, KubeObjectMenuRegistration, KubeObjectStatusRegistration, PageMenuRegistration, PageRegistration, StatusBarRegistration, } from "./registries";
 import type { Cluster } from "../main/cluster";
 import { LensExtension } from "./lens-extension";
 import { getExtensionPageUrl } from "./registries/page-registry";
 
-
 export class LensRendererExtension extends LensExtension {
   globalPages: PageRegistration[] = [];
   clusterPages: PageRegistration[] = [];
   globalPageMenus: PageMenuRegistration[] = [];
-  clusterPageMenus: PageMenuRegistration[] = [];
+  clusterPageMenus: ClusterPageMenuRegistration[] = [];
   kubeObjectStatusTexts: KubeObjectStatusRegistration[] = [];
   appPreferences: AppPreferenceRegistration[] = [];
   clusterFeatures: ClusterFeatureRegistration[] = [];

src/extensions/registries/__tests__/page-registry.test.ts

@@ -1,4 +1,4 @@
-import { getExtensionPageUrl, globalPageRegistry } from "../page-registry";
+import { getExtensionPageUrl, globalPageRegistry, PageParams } from "../page-registry";
 import { LensExtension } from "../../lens-extension";
 import React from "react";
 
@@ -17,6 +17,16 @@ describe("getPageUrl", () => {
       isBundled: false,
       isEnabled: true
     });
+    globalPageRegistry.add({
+      id: "page-with-params",
+      components: {
+        Page: () => React.createElement("Page with params")
+      },
+      params: {
+        test1: "test1-default",
+        test2: "" // no default value, just declaration
+      },
+    }, ext);
   });
 
   it("returns a page url for extension", () => {
@@ -34,6 +44,24 @@ describe("getPageUrl", () => {
   it("adds / prefix", () => {
     expect(getExtensionPageUrl({ extensionId: ext.name, pageId: "test" })).toBe("/extension/foo-bar/test");
   });
+
+  it("normalize possible multi-slashes in page.id", () => {
+    expect(getExtensionPageUrl({ extensionId: ext.name, pageId: "//test/" })).toBe("/extension/foo-bar/test");
+  });
+
+  it("gets page url with custom params", () => {
+    const params: PageParams<string> = { test1: "one", test2: "2" };
+    const searchParams = new URLSearchParams(params);
+    const pageUrl = getExtensionPageUrl({ extensionId: ext.name, pageId: "page-with-params", params });
+
+    expect(pageUrl).toBe(`/extension/foo-bar/page-with-params?${searchParams}`);
+  });
+
+  it("gets page url with default custom params", () => {
+    const defaultPageUrl = getExtensionPageUrl({ extensionId: ext.name, pageId: "page-with-params", });
+
+    expect(defaultPageUrl).toBe(`/extension/foo-bar/page-with-params?test1=test1-default`);
+  });
 });
 
 describe("globalPageRegistry", () => {
@@ -70,17 +98,17 @@ describe("globalPageRegistry", () => {
     ], ext);
   });
 
-  describe("getByPageMenuTarget", () => {
+  describe("getByPageTarget", () => {
     it("matching to first registered page without id", () => {
-      const page = globalPageRegistry.getByPageMenuTarget({ extensionId: ext.name });
+      const page = globalPageRegistry.getByPageTarget({ extensionId: ext.name });
 
       expect(page.id).toEqual(undefined);
       expect(page.extensionId).toEqual(ext.name);
-      expect(page.routePath).toEqual(getExtensionPageUrl({ extensionId: ext.name }));
+      expect(page.url).toEqual(getExtensionPageUrl({ extensionId: ext.name }));
     });
 
     it("returns matching page", () => {
-      const page = globalPageRegistry.getByPageMenuTarget({
+      const page = globalPageRegistry.getByPageTarget({
         pageId: "test-page",
         extensionId: ext.name
       });
@@ -89,7 +117,7 @@ describe("globalPageRegistry", () => {
     });
 
     it("returns null if target not found", () => {
-      const page = globalPageRegistry.getByPageMenuTarget({
+      const page = globalPageRegistry.getByPageTarget({
         pageId: "wrong-page",
         extensionId: ext.name
       });

src/extensions/registries/base-registry.ts

@@ -1,29 +1,34 @@
 // Base class for extensions-api registries
 import { action, observable } from "mobx";
 import { LensExtension } from "../lens-extension";
-import { rectify } from "../../common/utils";
 
-export class BaseRegistry<T> {
-  private items = observable<T>([], { deep: false });
+export class BaseRegistry<T, I = T> {
+  private items = observable.map<T, I>();
 
-  getItems(): T[] {
-    return this.items.toJS();
+  getItems(): I[] {
+    return Array.from(this.items.values());
   }
 
-  add(items: T | T[], ext?: LensExtension): () => void; // allow method overloading with required "ext"
   @action
-  add(items: T | T[]) {
-    const itemArray = rectify(items);
+  add(items: T | T[], extension?: LensExtension) {
+    const itemArray = [items].flat() as T[];
 
-    this.items.push(...itemArray);
+    itemArray.forEach(item => {
+      this.items.set(item, this.getRegisteredItem(item, extension));
+    });
 
     return () => this.remove(...itemArray);
   }
 
+  // eslint-disable-next-line unused-imports/no-unused-vars-ts
+  protected getRegisteredItem(item: T, extension?: LensExtension): I {
+    return item as any;
+  }
+
   @action
   remove(...items: T[]) {
     items.forEach(item => {
-      this.items.remove(item); // works because of {deep: false};
+      this.items.delete(item);
     });
   }
 }

src/extensions/registries/page-menu-registry.ts

@@ -1,19 +1,13 @@
 // Extensions-api -> Register page menu items
 import type { IconProps } from "../../renderer/components/icon";
 import type React from "react";
+import type { PageTarget, RegisteredPage } from "./page-registry";
 import { action } from "mobx";
 import { BaseRegistry } from "./base-registry";
 import { LensExtension } from "../lens-extension";
-import { RegisteredPage } from "./page-registry";
-
-export interface PageMenuTarget<P extends object = any> {
-  extensionId?: string;
-  pageId?: string;
-  params?: P;
-}
 
 export interface PageMenuRegistration {
-  target?: PageMenuTarget;
+  target?: PageTarget;
   title: React.ReactNode;
   components: PageMenuComponents;
 }
@@ -27,9 +21,9 @@ export interface PageMenuComponents {
   Icon: React.ComponentType<IconProps>;
 }
 
-export class GlobalPageMenuRegistry extends BaseRegistry<PageMenuRegistration> {
+export class PageMenuRegistry<T extends PageMenuRegistration> extends BaseRegistry<T> {
   @action
-  add(items: PageMenuRegistration[], ext: LensExtension) {
+  add(items: T[], ext: LensExtension) {
     const normalizedItems = items.map(menuItem => {
       menuItem.target = {
         extensionId: ext.name,
@@ -43,33 +37,25 @@ export class GlobalPageMenuRegistry extends BaseRegistry<PageMenuRegistration> {
   }
 }
 
-export class ClusterPageMenuRegistry extends BaseRegistry<ClusterPageMenuRegistration> {
-  @action
-  add(items: PageMenuRegistration[], ext: LensExtension) {
-    const normalizedItems = items.map(menuItem => {
-      menuItem.target = {
-        extensionId: ext.name,
-        ...(menuItem.target || {}),
-      };
-
-      return menuItem;
-    });
-
-    return super.add(normalizedItems);
-  }
-
+export class ClusterPageMenuRegistry extends PageMenuRegistry<ClusterPageMenuRegistration> {
   getRootItems() {
     return this.getItems().filter((item) => !item.parentId);
   }
 
   getSubItems(parent: ClusterPageMenuRegistration) {
-    return this.getItems().filter((item) => item.parentId === parent.id && item.target.extensionId === parent.target.extensionId);
+    return this.getItems().filter((item) => (
+      item.parentId === parent.id &&
+      item.target.extensionId === parent.target.extensionId
+    ));
   }
 
-  getByPage(page: RegisteredPage) {
-    return this.getItems().find((item) => item.target?.pageId == page.id && item.target?.extensionId === page.extensionId);
+  getByPage({ id: pageId, extensionId }: RegisteredPage) {
+    return this.getItems().find((item) => (
+      item.target.pageId == pageId &&
+      item.target.extensionId === extensionId
+    ));
   }
 }
 
-export const globalPageMenuRegistry = new GlobalPageMenuRegistry();
+export const globalPageMenuRegistry = new PageMenuRegistry();
 export const clusterPageMenuRegistry = new ClusterPageMenuRegistry();

src/extensions/registries/page-registry.ts

@@ -1,93 +1,120 @@
 // Extensions-api -> Custom page registration
-import type { PageMenuTarget } from "./page-menu-registry";
-import type React from "react";
-import path from "path";
-import { action } from "mobx";
-import { compile } from "path-to-regexp";
+
+import React from "react";
+import { observer } from "mobx-react";
 import { BaseRegistry } from "./base-registry";
 import { LensExtension, sanitizeExtensionName } from "../lens-extension";
-import logger from "../../main/logger";
-import { rectify } from "../../common/utils";
+import { PageParam, PageParamInit } from "../../renderer/navigation/page-param";
+import { createPageParam } from "../../renderer/navigation/helpers";
 
 export interface PageRegistration {
   /**
-   * Page ID or additional route path to indicate uniqueness within current extension registered pages
-   * Might contain special url placeholders, e.g. "/users/:userId?" (? - marks as optional param)
+   * Page ID, part of extension's page url, must be unique within same extension
    * When not provided, first registered page without "id" would be used for page-menus without target.pageId for same extension
    */
   id?: string;
-  /**
-   * Strict route matching to provided page-id, read also: https://reactrouter.com/web/api/NavLink/exact-bool
-   * In case when more than one page registered at same extension "pageId" is required to identify different pages,
-   * It might be useful to provide `exact: true` in some cases to avoid overlapping routes.
-   * Without {exact:true} second page never matches since first page-id/route already includes partial route.
-   * @example const pages = [
-   *  {id: "/users", exact: true},
-   *  {id: "/users/:userId?"}
-   * ]
-   * Pro-tip: registering pages in opposite order will make same effect without "exact".
-   */
-  exact?: boolean;
+  params?: PageParams<string | ExtensionPageParamInit>;
   components: PageComponents;
 }
 
-export interface RegisteredPage extends PageRegistration {
-  extensionId: string; // required for compiling registered page to url with page-menu-target to compare
-  routePath: string; // full route-path to registered extension page
-}
+// exclude "name" field since provided as key in page.params
+export type ExtensionPageParamInit = Omit<PageParamInit, "name" | "isSystem">;
 
 export interface PageComponents {
   Page: React.ComponentType<any>;
 }
 
-export function getExtensionPageUrl<P extends object>({ extensionId, pageId = "", params }: PageMenuTarget<P>): string {
-  const extensionBaseUrl = compile(`/extension/:name`)({
-    name: sanitizeExtensionName(extensionId), // compile only with extension-id first and define base path
-  });
-  const extPageRoutePath = path.posix.join(extensionBaseUrl, pageId);
-
-  if (params) {
-    return compile(extPageRoutePath)(params); // might throw error when required params not passed
-  }
-
-  return extPageRoutePath;
+export interface PageTarget<P = PageParams> {
+  extensionId?: string;
+  pageId?: string;
+  params?: P;
 }
 
-export class PageRegistry extends BaseRegistry<RegisteredPage> {
-  @action
-  add(items: PageRegistration | PageRegistration[], ext: LensExtension) {
-    const itemArray = rectify(items);
-    let registeredPages: RegisteredPage[] = [];
+export interface PageParams<V = any> {
+  [paramName: string]: V;
+}
 
-    try {
-      registeredPages = itemArray.map(page => ({
-        ...page,
-        extensionId: ext.name,
-        routePath: getExtensionPageUrl({ extensionId: ext.name, pageId: page.id }),
-      }));
-    } catch (err) {
-      logger.error(`[EXTENSION]: page-registration failed`, {
-        items,
-        extension: ext,
-        error: String(err),
-      });
+export interface PageComponentProps<P extends PageParams = {}> {
+  params?: {
+    [N in keyof P]: PageParam<P[N]>;
+  }
+}
+
+export interface RegisteredPage {
+  id: string;
+  extensionId: string;
+  url: string; // registered extension's page URL (without page params)
+  params: PageParams<PageParam>; // normalized params
+  components: PageComponents; // normalized components
+}
+
+export function getExtensionPageUrl(target: PageTarget): string {
+  const { extensionId, pageId = "", params: targetParams = {} } = target;
+
+  const pagePath = ["/extension", sanitizeExtensionName(extensionId), pageId]
+    .filter(Boolean)
+    .join("/").replace(/\/+/g, "/").replace(/\/$/, ""); // normalize multi-slashes (e.g. coming from page.id)
+
+  const pageUrl = new URL(pagePath, `http://localhost`);
+
+  // stringify params to matched target page
+  const registeredPage = globalPageRegistry.getByPageTarget(target) || clusterPageRegistry.getByPageTarget(target);
+
+  if (registeredPage?.params) {
+    Object.entries(registeredPage.params).forEach(([name, param]) => {
+      const paramValue = param.stringify(targetParams[name]);
+
+      if (param.init.skipEmpty && param.isEmpty(paramValue)) {
+        pageUrl.searchParams.delete(name);
+      } else {
+        pageUrl.searchParams.set(name, paramValue);
+      }
+    });
+  }
+
+  return pageUrl.href.replace(pageUrl.origin, "");
+}
+
+export class PageRegistry extends BaseRegistry<PageRegistration, RegisteredPage> {
+  protected getRegisteredItem(page: PageRegistration, ext: LensExtension): RegisteredPage {
+    const { id: pageId } = page;
+    const extensionId = ext.name;
+    const params = this.normalizeParams(page.params);
+    const components = this.normalizeComponents(page.components, params);
+    const url = getExtensionPageUrl({ extensionId, pageId });
+
+    return {
+      id: pageId, extensionId, params, components, url,
+    };
+  }
+
+  protected normalizeComponents(components: PageComponents, params?: PageParams<PageParam>): PageComponents {
+    if (params) {
+      const { Page } = components;
+
+      components.Page = observer((props: object) => React.createElement(Page, { params, ...props }));
     }
 
-    return super.add(registeredPages);
+    return components;
   }
 
-  getUrl<P extends object>({ extensionId, id: pageId }: RegisteredPage, params?: P) {
-    return getExtensionPageUrl({ extensionId, pageId, params });
+  protected normalizeParams(params?: PageParams<string | ExtensionPageParamInit>): PageParams<PageParam> {
+    if (!params) {
+      return;
+    }
+    Object.entries(params).forEach(([name, value]) => {
+      const paramInit: PageParamInit = typeof value === "object"
+        ? { name, ...value }
+        : { name, defaultValue: value };
+
+      params[paramInit.name] = createPageParam(paramInit);
+    });
+
+    return params as PageParams<PageParam>;
   }
 
-  getByPageMenuTarget(target: PageMenuTarget = {}): RegisteredPage | null {
-    const targetUrl = getExtensionPageUrl(target);
-
-    return this.getItems().find(({ id: pageId, extensionId }) => {
-      const pageUrl = getExtensionPageUrl({ extensionId, pageId, params: target.params }); // compiled with provided params
-
-      return targetUrl === pageUrl;
-    }) || null;
+  getByPageTarget(target: PageTarget): RegisteredPage | null {
+    return this.getItems().find(page => page.extensionId === target.extensionId && page.id === target.pageId) || null;
   }
 }
 

src/extensions/renderer-api/navigation.ts

@@ -1,3 +1,12 @@
-export { navigate } from "../../renderer/navigation";
-export { hideDetails, showDetails, getDetailsUrl } from "../../renderer/navigation";
+import { PageParam, PageParamInit } from "../../renderer/navigation/page-param";
+import { navigation } from "../../renderer/navigation";
+
+export type { PageParamInit, PageParam } from "../../renderer/navigation/page-param";
+export { navigate, isActiveRoute } from "../../renderer/navigation/helpers";
+export { hideDetails, showDetails, getDetailsUrl } from "../../renderer/components/kube-object/kube-object-details";
 export { IURLParams } from "../../common/utils/buildUrl";
+
+// exporting to extensions-api version of helper without `isSystem` flag
+export function createPageParam<V = string>(init: PageParamInit<V>) {
+  return new PageParam<V>(init, navigation);
+}