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

Remove extension when folder is removed during runtime #1518

Merged
merged 1 commit into from Nov 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Expand Up @@ -362,6 +362,7 @@
"nodemon": "^2.0.4",
"patch-package": "^6.2.2",
"postinstall-postinstall": "^2.1.0",
"prettier": "^2.2.0",
"progress-bar-webpack-plugin": "^2.1.0",
"raw-loader": "^4.0.1",
"react": "^16.14.0",
Expand Down
2 changes: 1 addition & 1 deletion src/common/utils/rectify-array.ts
Expand Up @@ -3,6 +3,6 @@
* @param items either one item or an array of items
* @returns a list of items
*/
export function recitfy<T>(items: T | T[]): T[] {
export function rectify<T>(items: T | T[]): T[] {
return Array.isArray(items) ? items : [items];
}
120 changes: 120 additions & 0 deletions src/extensions/__tests__/extension-loader.test.ts
@@ -0,0 +1,120 @@
import { ExtensionLoader } from "../extension-loader";

const manifestPath = "manifest/path";
const manifestPath2 = "manifest/path2";
const manifestPath3 = "manifest/path3";

jest.mock(
"electron",
() => ({
ipcRenderer: {
invoke: jest.fn(async (channel: string, ...args: any[]) => {
if (channel === "extensions:loaded") {
return [
[
manifestPath,
{
manifest: {
name: "TestExtension",
version: "1.0.0",
},
manifestPath,
isBundled: false,
isEnabled: true,
},
],
[
manifestPath2,
{
manifest: {
name: "TestExtension2",
version: "2.0.0",
},
manifestPath: manifestPath2,
isBundled: false,
isEnabled: true,
},
],
];
}
}),
on: jest.fn(
(channel: string, listener: (event: any, ...args: any[]) => void) => {
if (channel === "extensions:loaded") {
// First initialize with extensions 1 and 2
// and then broadcast event to remove extensioin 2 and add extension number 3
setTimeout(() => {
listener({}, [
[
manifestPath,
{
manifest: {
name: "TestExtension",
version: "1.0.0",
},
manifestPath,
isBundled: false,
isEnabled: true,
},
],
[
manifestPath3,
{
manifest: {
name: "TestExtension3",
version: "3.0.0",
},
manifestPath3,
isBundled: false,
isEnabled: true,
},
],
]);
}, 10);
}
}
),
},
}),
{
virtual: true,
}
);

describe("ExtensionLoader", () => {
it("renderer updates extension after ipc broadcast", async (done) => {
const extensionLoader = new ExtensionLoader();

expect(extensionLoader.userExtensions).toMatchInlineSnapshot(`Map {}`);

await extensionLoader.init();

setTimeout(() => {
// Assert the extensions after the extension broadcast event
expect(extensionLoader.userExtensions).toMatchInlineSnapshot(`
Map {
"manifest/path" => Object {
"isBundled": false,
"isEnabled": true,
"manifest": Object {
"name": "TestExtension",
"version": "1.0.0",
},
"manifestPath": "manifest/path",
},
"manifest/path3" => Object {
"isBundled": false,
"isEnabled": true,
"manifest": Object {
"name": "TestExtension3",
"version": "3.0.0",
},
"manifestPath3": "manifest/path3",
},
}
`);

done();
}, 10);
});
});
124 changes: 95 additions & 29 deletions src/extensions/extension-loader.ts
@@ -1,4 +1,5 @@
import { app, ipcRenderer, remote } from "electron";
import { EventEmitter } from "events";
import { action, computed, observable, reaction, toJS, when } from "mobx";
import path from "path";
import { getHostedCluster } from "../common/cluster-store";
Expand Down Expand Up @@ -26,26 +27,32 @@ export class ExtensionLoader {
protected instances = observable.map<LensExtensionId, LensExtension>();
protected readonly requestExtensionsChannel = "extensions:loaded";

// emits event "remove" of type LensExtension when the extension is removed
private events = new EventEmitter();

@observable isLoaded = false;
whenLoaded = when(() => this.isLoaded);

@computed get userExtensions(): Map<LensExtensionId, InstalledExtension> {
const extensions = this.extensions.toJS();

extensions.forEach((ext, extId) => {
if (ext.isBundled) {
extensions.delete(extId);
}
});

return extensions;
}

@action
async init() {
if (ipcRenderer) {
this.initRenderer();
await this.initRenderer();
} else {
this.initMain();
await this.initMain();
}

extensionsStore.manageState(this);
}

Expand All @@ -57,11 +64,28 @@ export class ExtensionLoader {
this.extensions.set(extension.manifestPath as LensExtensionId, extension);
}

removeInstance(lensExtensionId: LensExtensionId) {
logger.info(`${logModule} deleting extension instance ${lensExtensionId}`);
const instance = this.instances.get(lensExtensionId);

if (instance) {
try {
instance.disable();
this.events.emit("remove", instance);
this.instances.delete(lensExtensionId);
} catch (error) {
logger.error(`${logModule}: deactivation extension error`, { lensExtensionId, error });
}
}
}
Comment on lines +71 to +80
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about the if (!instance) { return; } style instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see that being necessarily better, but fixed


removeExtension(lensExtensionId: LensExtensionId) {
// TODO: Remove the extension properly (from menus etc.)
this.removeInstance(lensExtensionId);

if (!this.extensions.delete(lensExtensionId)) {
throw new Error(`Can't remove extension ${lensExtensionId}, doesn't exist.`);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

New line at the end of a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

}

protected async initMain() {
Expand All @@ -79,14 +103,25 @@ export class ExtensionLoader {
}

protected async initRenderer() {
const extensionListHandler = ( extensions: [LensExtensionId, InstalledExtension][]) => {
const extensionListHandler = (extensions: [LensExtensionId, InstalledExtension][]) => {
this.isLoaded = true;
const receivedExtensionIds = extensions.map(([lensExtensionId]) => lensExtensionId);

// Add new extensions
extensions.forEach(([extId, ext]) => {
if (!this.extensions.has(extId)) {
this.extensions.set(extId, ext);
}
});

// Remove deleted extensions
this.extensions.forEach((_, lensExtensionId) => {
if (!receivedExtensionIds.includes(lensExtensionId)) {
this.removeExtension(lensExtensionId);
}
});
};

requestMain(this.requestExtensionsChannel).then(extensionListHandler);
subscribeToBroadcast(this.requestExtensionsChannel, (event, extensions: [LensExtensionId, InstalledExtension][]) => {
extensionListHandler(extensions);
Expand All @@ -95,36 +130,73 @@ export class ExtensionLoader {

loadOnMain() {
logger.info(`${logModule}: load on main`);
this.autoInitExtensions(async (ext: LensMainExtension) => [
registries.menuRegistry.add(ext.appMenus)
]);
this.autoInitExtensions(async (extension: LensMainExtension) => {
// Each .add returns a function to remove the item
const removeItems = [
registries.menuRegistry.add(extension.appMenus)
];

this.events.on("remove", (removedExtension: LensRendererExtension) => {
// manifestPath is considered the id
if (removedExtension.manifestPath === extension.manifestPath) {
removeItems.forEach(remove => {
remove();
});
}
});

return removeItems;
});
}

loadOnClusterManagerRenderer() {
logger.info(`${logModule}: load on main renderer (cluster manager)`);
this.autoInitExtensions(async (ext: LensRendererExtension) => [
registries.globalPageRegistry.add(ext.globalPages, ext),
registries.globalPageMenuRegistry.add(ext.globalPageMenus, ext),
registries.appPreferenceRegistry.add(ext.appPreferences),
registries.clusterFeatureRegistry.add(ext.clusterFeatures),
registries.statusBarRegistry.add(ext.statusBarItems),
]);
this.autoInitExtensions(async (extension: LensRendererExtension) => {
const removeItems = [
registries.globalPageRegistry.add(extension.globalPages, extension),
registries.globalPageMenuRegistry.add(extension.globalPageMenus, extension),
registries.appPreferenceRegistry.add(extension.appPreferences),
registries.clusterFeatureRegistry.add(extension.clusterFeatures),
registries.statusBarRegistry.add(extension.statusBarItems),
];

this.events.on("remove", (removedExtension: LensRendererExtension) => {
if (removedExtension.manifestPath === extension.manifestPath) {
removeItems.forEach(remove => {
remove();
});
}
});

return removeItems;
});
}

loadOnClusterRenderer() {
logger.info(`${logModule}: load on cluster renderer (dashboard)`);
const cluster = getHostedCluster();
this.autoInitExtensions(async (ext: LensRendererExtension) => {
if (await ext.isEnabledForCluster(cluster) === false) {
this.autoInitExtensions(async (extension: LensRendererExtension) => {
if (await extension.isEnabledForCluster(cluster) === false) {
return [];
}
return [
registries.clusterPageRegistry.add(ext.clusterPages, ext),
registries.clusterPageMenuRegistry.add(ext.clusterPageMenus, ext),
registries.kubeObjectMenuRegistry.add(ext.kubeObjectMenuItems),
registries.kubeObjectDetailRegistry.add(ext.kubeObjectDetailItems),
registries.kubeObjectStatusRegistry.add(ext.kubeObjectStatusTexts)

const removeItems = [
registries.clusterPageRegistry.add(extension.clusterPages, extension),
registries.clusterPageMenuRegistry.add(extension.clusterPageMenus, extension),
registries.kubeObjectMenuRegistry.add(extension.kubeObjectMenuItems),
registries.kubeObjectDetailRegistry.add(extension.kubeObjectDetailItems),
registries.kubeObjectStatusRegistry.add(extension.kubeObjectStatusTexts)
];

this.events.on("remove", (removedExtension: LensRendererExtension) => {
if (removedExtension.manifestPath === extension.manifestPath) {
removeItems.forEach(remove => {
remove();
});
}
});

return removeItems;
});
}

Expand All @@ -148,13 +220,7 @@ export class ExtensionLoader {
logger.error(`${logModule}: activation extension error`, { ext, err });
}
} else if (!ext.isEnabled && alreadyInit) {
try {
const instance = this.instances.get(extId);
instance.disable();
this.instances.delete(extId);
} catch (err) {
logger.error(`${logModule}: deactivation extension error`, { ext, err });
}
this.removeInstance(extId);
}
}
}, {
Expand Down
4 changes: 2 additions & 2 deletions src/extensions/registries/base-registry.ts
@@ -1,7 +1,7 @@
// Base class for extensions-api registries
import { action, observable } from "mobx";
import { LensExtension } from "../lens-extension";
import { recitfy } from "../../common/utils";
import { rectify } from "../../common/utils";

export class BaseRegistry<T> {
private items = observable<T>([], { deep: false });
Expand All @@ -13,7 +13,7 @@ export class BaseRegistry<T> {
add(items: T | T[], ext?: LensExtension): () => void; // allow method overloading with required "ext"
@action
add(items: T | T[]) {
const itemArray = recitfy(items);
const itemArray = rectify(items);
this.items.push(...itemArray);
return () => this.remove(...itemArray);
}
Expand Down
4 changes: 2 additions & 2 deletions src/extensions/registries/page-registry.ts
Expand Up @@ -7,7 +7,7 @@ import { compile } from "path-to-regexp";
import { BaseRegistry } from "./base-registry";
import { LensExtension, sanitizeExtensionName } from "../lens-extension";
import logger from "../../main/logger";
import { recitfy } from "../../common/utils";
import { rectify } from "../../common/utils";

export interface PageRegistration {
/**
Expand Down Expand Up @@ -54,7 +54,7 @@ export function getExtensionPageUrl<P extends object>({ extensionId, pageId = ""
export class PageRegistry extends BaseRegistry<RegisteredPage> {
@action
add(items: PageRegistration | PageRegistration[], ext: LensExtension) {
const itemArray = recitfy(items);
const itemArray = rectify(items);
let registeredPages: RegisteredPage[] = [];
try {
registeredPages = itemArray.map(page => ({
Expand Down
2 changes: 2 additions & 0 deletions src/renderer/components/+extensions/extensions.tsx
Expand Up @@ -303,6 +303,7 @@ export class Extensions extends React.Component {

renderExtensions() {
const { extensions, extensionsPath, search } = this;

if (!extensions.length) {
return (
<div className="flex align-center box grow justify-center gaps">
Expand All @@ -311,6 +312,7 @@ export class Extensions extends React.Component {
</div>
);
}

return extensions.map(ext => {
const { manifestPath: extId, isEnabled, manifest } = ext;
const { name, description } = manifest;
Expand Down