Skip to content
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

Table component injection tokens #7754

Merged
merged 20 commits into from
May 26, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions open-lens/src/renderer/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ import {
rendererExtensionApi as Renderer,
commonExtensionApi as Common,
registerLensCore,
metricsFeature
metricsFeature,
tableFeature,
} from "@k8slens/core/renderer";
import { autoRegister } from "@ogre-tools/injectable-extension-for-auto-registration";
import { registerFeature } from "@k8slens/feature-core";
Expand Down Expand Up @@ -49,7 +50,8 @@ runInAction(() => {
keyboardShortcutsFeature,
reactApplicationFeature,
routingFeature,
metricsFeature
metricsFeature,
tableFeature,
);

autoRegister({
Expand Down
18 changes: 18 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,7 @@
"@k8slens/routing": "^1.0.0-alpha.5",
"@k8slens/run-many": "^1.0.0-alpha.1",
"@k8slens/startable-stoppable": "^1.0.0-alpha.1",
"@k8slens/table-tokens": "^6.5.0-alpha.7",
"@k8slens/test-utils": "^1.0.0-alpha.3",
"@k8slens/tooltip": "^1.0.0-alpha.5",
"@k8slens/utilities": "^1.0.0-alpha.1",
Expand Down
15 changes: 15 additions & 0 deletions packages/core/src/features/table/table-component.injectable.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this under open-lens package and import Table from @k8slens/core. Add export if it's not exported yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

* Copyright (c) OpenLens Authors. All rights reserved.
* Licensed under MIT License. See LICENSE in root directory for more information.
*/
import { tableComponentInjectionToken } from "@k8slens/table-tokens";
import { getInjectable } from "@ogre-tools/injectable";
import { Table } from "../../renderer/components/table/table";

const tableComponentInjectable = getInjectable({
id: "table-component",
instantiate: () => ({ Component: Table }),
injectionToken: tableComponentInjectionToken,
});

export default tableComponentInjectable;
14 changes: 14 additions & 0 deletions packages/core/src/features/table/table-feature.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/**
* Copyright (c) OpenLens Authors. All rights reserved.
* Licensed under MIT License. See LICENSE in root directory for more information.
*/
import { getFeature } from "@k8slens/feature-core";
import tableComponentInjectable from "./table-component.injectable";

export const tableFeature = getFeature({
id: "core-table-feature",

register: (di) => {
di.register(tableComponentInjectable);
},
});
Comment on lines +8 to +14
Copy link
Contributor

@ixrock ixrock May 25, 2023

Choose a reason for hiding this comment

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

Can you remind me (mostly for learning purpose) why do we need to make it as a feature?
(instead of simple some injection token re-use from some package, e.g. import {cystomTableToken} from "@k8slens/item-list")?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding, it seems to be an architectural decision. If we utilize an injection token in the core and there are two "users" of this token registered, an error will occur. Only one injection token consumer is allowed. However, by incorporating a feature within the open-lens package, we can easily separate features to determine their visibility in all lens variations. Additionally, the feature is beneficial when dealing with multiple injection tokens that need to be registered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jansav @Iku-turso A good chance to clarify the question and correct me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, the feature is beneficial when dealing with multiple injection tokens that need to be registered.

Can't get it.. why not di.injectMany(customTableToken)? And decide which one to use..
This is simple & perfect for plain component injections/replacement).

Copy link
Contributor

Choose a reason for hiding this comment

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

Feature is a group of injectables which are registered and deregistered at once. The idea is that everything related to the feature can be in separate npm package called @k8slens/some-feature which then exports someFeature. Now that open-lens (or any of the applications) registers someFeature with registerFeature, the feature is added.

--

This case seems to have been built in a way that we expect a single implementation of tableComponentInjectionToken to be around. This makes sense, because there should be only one of them. However, the important part here is that implementation tableComponentInjectable should be under open-lens and not under core. Therefore it's only registered in the open-lens and not in any other application which is built on top of core package. In this way di.inject(tableComponentInjectionToken) will be enough. Feature is not needed here because the "open-lens" implementation of table can be moved under open-lens and we're fine with stuff under there being registered individually.

@aleksfront @ixrock

Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ import { Cluster } from "../../../../common/cluster/cluster";
import hostedClusterInjectable from "../../../cluster-frame-context/hosted-cluster.injectable";
import userPreferencesStateInjectable from "../../../../features/user-preferences/common/state.injectable";
import type { DiContainer } from "@ogre-tools/injectable";
import { registerFeature } from "@k8slens/feature-core";
import { runInAction } from "mobx";
import { tableFeature } from "../../../library";

describe("<PodDisruptionBudgets />", () => {
let di: DiContainer;
Expand Down Expand Up @@ -69,6 +72,10 @@ describe("<PodDisruptionBudgets />", () => {
}),
} as any,
}));

runInAction(() => {
registerFeature(di, tableFeature);
});
});

describe("PDB with minAvailable 0", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { computed, makeObservable } from "mobx";
import { Observer, observer } from "mobx-react";
import type { ConfirmDialogParams } from "../confirm-dialog";
import type { TableProps, TableRowProps, TableSortCallbacks } from "../table";
import { Table, TableCell, TableHead, TableRow } from "../table";
import { TableCell, TableHead, TableRow } from "../table";
import type { IClassName, StrictReactNode } from "@k8slens/utilities";
import { cssNames, isDefined, isReactNode, noop, prevDefault, stopPropagation } from "@k8slens/utilities";
import type { AddRemoveButtonsProps } from "../add-remove-buttons";
Expand All @@ -35,6 +35,8 @@ import type { ToggleTableColumnVisibility } from "../../../features/user-prefere
import toggleTableColumnVisibilityInjectable from "../../../features/user-preferences/common/toggle-table-column-visibility.injectable";
import type { IsTableColumnHidden } from "../../../features/user-preferences/common/is-table-column-hidden.injectable";
import isTableColumnHiddenInjectable from "../../../features/user-preferences/common/is-table-column-hidden.injectable";
import type { TableComponent, TableDataContextValue } from "@k8slens/table-tokens";
import { TableDataContext, tableComponentInjectionToken } from "@k8slens/table-tokens";

export interface ItemListLayoutContentProps<Item extends ItemObject, PreLoadStores extends boolean> {
getFilters: () => Filter[];
Expand Down Expand Up @@ -79,13 +81,17 @@ interface Dependencies {
openConfirmDialog: OpenConfirmDialog;
toggleTableColumnVisibility: ToggleTableColumnVisibility;
isTableColumnHidden: IsTableColumnHidden;
table: TableComponent;
}

@observer
class NonInjectedItemListLayoutContent<
Item extends ItemObject,
PreLoadStores extends boolean,
> extends React.Component<ItemListLayoutContentProps<Item, PreLoadStores> & Dependencies> {
static contextType = TableDataContext;
declare context: TableDataContextValue;

constructor(props: ItemListLayoutContentProps<Item, PreLoadStores> & Dependencies) {
super(props);
makeObservable(this);
Expand Down Expand Up @@ -299,6 +305,7 @@ class NonInjectedItemListLayoutContent<
const {
store, hasDetailsView, addRemoveButtons = {}, virtual, sortingCallbacks,
detailsItem, className, tableProps = {}, tableId, getItems, activeTheme,
table,
} = this.props;
const selectedItemId = detailsItem && detailsItem.getId();
const classNames = cssNames(className, "box", "grow", activeTheme.get().type);
Expand All @@ -307,8 +314,9 @@ class NonInjectedItemListLayoutContent<

return (
<div className="items box grow flex column">
<Table
<table.Component
tableId={tableId}
columns={this.context.columns}
virtual={virtual}
selectable={hasDetailsView}
sortable={sortingCallbacks}
Expand All @@ -322,7 +330,7 @@ class NonInjectedItemListLayoutContent<
>
{this.renderTableHeader()}
{this.renderItems()}
</Table>
</table.Component>

<Observer>
{() => (
Expand Down Expand Up @@ -385,5 +393,6 @@ export const ItemListLayoutContent = withInjectables<Dependencies, ItemListLayou
openConfirmDialog: di.inject(openConfirmDialogInjectable),
toggleTableColumnVisibility: di.inject(toggleTableColumnVisibilityInjectable),
isTableColumnHidden: di.inject(isTableColumnHiddenInjectable),
table: di.inject(tableComponentInjectionToken),
}),
}) as <Item extends ItemObject, PreLoadStores extends boolean>(props: ItemListLayoutContentProps<Item, PreLoadStores>) => React.ReactElement;
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ import type { PodStore } from "../workloads-pods/store";
import { Cluster } from "../../../common/cluster/cluster";
import isTableColumnHiddenInjectable from "../../../features/user-preferences/common/is-table-column-hidden.injectable";
import { podListLayoutColumnInjectionToken } from "@k8slens/list-layout";
import { registerFeature } from "@k8slens/feature-core";
import { runInAction } from "mobx";
import { tableFeature } from "../../library";

describe("kube-object-list-layout", () => {
let di: DiContainer;
Expand Down Expand Up @@ -54,6 +57,10 @@ describe("kube-object-list-layout", () => {
get: () => ({}),
}));

runInAction(() => {
registerFeature(di, tableFeature);
});

podStore = di.inject(podStoreInjectable);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import type { ClusterContext } from "../../cluster-frame-context/cluster-frame-c
import type { GeneralKubeObjectListLayoutColumn, SpecificKubeListLayoutColumn } from "@k8slens/list-layout";
import { kubeObjectListLayoutColumnInjectionToken } from "@k8slens/list-layout";
import { sortBy } from "lodash";
import { TableDataContext } from "@k8slens/table-tokens";

export type KubeItemListStore<K extends KubeObject> = ItemListStore<K, false> & SubscribableStore & {
getByPath: (path: string) => K | undefined;
Expand Down Expand Up @@ -186,7 +187,7 @@ class NonInjectedKubeObjectListLayout<
...targetColumns,
], (v) => -v.priority).map((col) => col.header);

return (
const itemsListLayout = (
<ItemListLayout<K, false>
className={cssNames("KubeObjectListLayout", className)}
store={store}
Expand Down Expand Up @@ -233,6 +234,15 @@ class NonInjectedKubeObjectListLayout<
{...layoutProps}
/>
);

return (
<TableDataContext.Provider
value={{
columns: targetColumns as GeneralKubeObjectListLayoutColumn[],
}}>
{itemsListLayout}
</TableDataContext.Provider>
);
ixrock marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ import { sendMessageToChannelInjectionToken } from "@k8slens/messaging";
import { getMessageBridgeFake } from "@k8slens/messaging-fake-bridge";
import { historyInjectionToken } from "@k8slens/routing";
import writeJsonSyncInjectable from "../../../common/fs/write-json-sync.injectable";
import { tableFeature } from "../../library";

type MainDiCallback = (container: { mainDi: DiContainer }) => void | Promise<void>;
type WindowDiCallback = (container: { windowDi: DiContainer }) => void | Promise<void>;
Expand Down Expand Up @@ -256,6 +257,7 @@ export const getApplicationBuilder = () => {
registerFeature(
windowDi,
applicationFeature,
tableFeature,
);

windowDi.register(rendererExtensionsStateInjectable);
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/renderer/library.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,4 @@ export * as ReactRouterDom from "react-router-dom";
export * as rendererExtensionApi from "../extensions/renderer-api";
export * as commonExtensionApi from "../extensions/common-api";
export { metricsFeature } from "../features/metrics/metrics-feature";
export { tableFeature } from "../features/table/table-feature";
5 changes: 4 additions & 1 deletion packages/logger/src/logger.injectable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,10 @@ export const logSillyInjectionToken = getInjectionToken<LogFunction>({

const screamingKebabCase = (str: string) => pipeline(str, kebabCase, toUpper);

const getLogFunctionFor = (scenario: keyof Logger, namespace: string | undefined) => {
const getLogFunctionFor = (
scenario: keyof Logger,
namespace: string | undefined
) => {
const prefix = namespace
? `[${screamingKebabCase(namespace.replace(/-feature$/, ""))}]: `
: "";
Expand Down
13 changes: 7 additions & 6 deletions packages/logger/src/logger.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ describe("logger", () => {
instantiate: (di) => di.inject(injectionToken),
});


const someFeature = getFeature({
id: "some-feature",

Expand Down Expand Up @@ -124,11 +123,13 @@ describe("logger", () => {

registerFeature(di, loggerFeature);

di.register(getInjectable({
id: "some-transport",
instantiate: () => new TransportStream({ log }),
injectionToken: loggerTransportInjectionToken,
}))
di.register(
getInjectable({
id: "some-transport",
instantiate: () => new TransportStream({ log }),
injectionToken: loggerTransportInjectionToken,
})
);

const logger = di.inject(loggerInjectable);

Expand Down
8 changes: 5 additions & 3 deletions packages/logger/src/transports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
import { getInjectionToken } from "@ogre-tools/injectable";
import type TransportStream from "winston-transport";

export const loggerTransportInjectionToken = getInjectionToken<TransportStream>({
id: "logger-transport",
});
export const loggerTransportInjectionToken = getInjectionToken<TransportStream>(
{
id: "logger-transport",
}
);
3 changes: 3 additions & 0 deletions packages/table/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Description

The package exports tokens needed for external table configuration.
37 changes: 37 additions & 0 deletions packages/table/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { getInjectionToken } from "@ogre-tools/injectable";
import type { KubeObject } from "@k8slens/kube-object/src/kube-object";
import type {
BaseKubeObjectListLayoutColumn,
GeneralKubeObjectListLayoutColumn,
SpecificKubeListLayoutColumn,
} from "@k8slens/list-layout";
import React from "react";

type Column = (
| BaseKubeObjectListLayoutColumn<KubeObject>
| SpecificKubeListLayoutColumn<KubeObject>
| GeneralKubeObjectListLayoutColumn
);

export interface TableComponentProps {
tableId?: string;
columns?: Column[];
save?: (state: object) => void;
load?: (tableId: string) => object;
}

export interface TableDataContextValue {
columns?: Column[];
}

export const TableDataContext = React.createContext<TableDataContextValue>({
columns: [],
});

export interface TableComponent {
Component: React.ComponentType<TableComponentProps>;
}

export const tableComponentInjectionToken = getInjectionToken<TableComponent>({
id: "table-component-injection-token",
});
29 changes: 29 additions & 0 deletions packages/table/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
{
"name": "@k8slens/table-tokens",
"version": "6.5.0-alpha.7",
"description": "Injection token exporter for table components",
"license": "MIT",
"type": "commonjs",
"private": false,
"publishConfig": {
"access": "public",
"registry": "https://registry.npmjs.org/"
},
"main": "./dist/index.js",
"types": "./dist/index.d.ts",
"files": [
"dist"
],
"scripts": {
"clean": "rimraf dist/",
"build": "lens-webpack-build"
},
"devDependencies": {
"@k8slens/webpack": "^6.5.0-alpha.8",
"rimraf": "^4.4.1"
},
"peerDependencies": {
"@ogre-tools/injectable": "^16.1.0",
"react": "^17.0.2"
}
}
4 changes: 4 additions & 0 deletions packages/table/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"extends": "@k8slens/typescript/config/base.json",
"include": ["**/*.ts"]
}
1 change: 1 addition & 0 deletions packages/table/webpack.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = require("@k8slens/webpack").configForNode;