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.

Sign up for GitHub

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

Extensions api fixes #1233

Merged
merged 5 commits into from
Nov 7, 2020
Merged

Extensions api fixes #1233

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
abaacdd
fix: create extension instance only when enabled
ixrock Nov 5, 2020
4cb4f5a
mark extension.isEnabled with private modifier
ixrock Nov 5, 2020
1057042
try-catch errors for extension.disable()
ixrock Nov 6, 2020
5643e84
fixes & refactoring
ixrock Nov 7, 2020
9a0378f
make ext.isBundled non optional
ixrock Nov 7, 2020
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
75 changes: 55 additions & 20 deletions src/extensions/extension-loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@ import type { LensRendererExtension } from "./lens-renderer-extension"
import type { InstalledExtension } from "./extension-manager";
import path from "path"
import { broadcastIpc } from "../common/ipc"
import { computed, observable, reaction, when } from "mobx"
import { action, computed, observable, reaction, toJS, when } from "mobx"
import logger from "../main/logger"
import { app, ipcRenderer, remote } from "electron"
import * as registries from "./registries";
import { extensionsStore } from "./extensions-store";

// lazy load so that we get correct userData
export function extensionPackagesRoot() {
Expand All @@ -16,8 +17,8 @@ export function extensionPackagesRoot() {

export class ExtensionLoader {
@observable isLoaded = false;
protected extensions = observable.map<LensExtensionId, InstalledExtension>([], { deep: false });
protected instances = observable.map<LensExtensionId, LensExtension>([], { deep: false })
protected extensions = observable.map<LensExtensionId, InstalledExtension>();
protected instances = observable.map<LensExtensionId, LensExtension>()

constructor() {
if (ipcRenderer) {
Expand All @@ -30,18 +31,39 @@ export class ExtensionLoader {
})
});
}
this.manageExtensionsState();
}

@computed get userExtensions(): LensExtension[] {
return [...this.instances.values()].filter(ext => !ext.isBundled)
@computed get userExtensions(): InstalledExtension[] {
return Array.from(this.toJSON().values()).filter(ext => !ext.isBundled)
}

async init() {
const { extensionManager } = await import("./extension-manager");
const installedExtensions = await extensionManager.load();
this.extensions.replace(installedExtensions);
protected async manageExtensionsState() {
await extensionsStore.whenLoaded;
await when(() => this.isLoaded);

// apply initial state
this.extensions.forEach((ext, extId) => {
ext.enabled = ext.isBundled || extensionsStore.isEnabled(extId);
})

// handle updated state from store
reaction(() => extensionsStore.extensions.toJS(), extensionsState => {
extensionsState.forEach((state, extId) => {
const ext = this.extensions.get(extId);
if (ext && !ext.isBundled && ext.enabled !== state.enabled) {
ext.enabled = state.enabled;
}
})
});
}

@action
async init(extensions: Map<LensExtensionId, InstalledExtension>) {
this.extensions.replace(extensions);
this.isLoaded = true;
this.loadOnMain();
this.broadcastExtensions();
}

loadOnMain() {
Expand Down Expand Up @@ -71,21 +93,27 @@ export class ExtensionLoader {
}

protected autoInitExtensions(register: (ext: LensExtension) => Function[]) {
return reaction(() => this.extensions.toJS(), (installedExtensions) => {
for (const [id, ext] of installedExtensions) {
let instance = this.instances.get(ext.manifestPath)
if (!instance) {
const extensionModule = this.requireExtension(ext)
if (!extensionModule) {
continue
}
return reaction(() => this.toJSON(), (installedExtensions) => {
for (const [extId, ext] of installedExtensions) {
let instance = this.instances.get(extId);
if (ext.enabled && !instance) {
try {
const extensionModule = this.requireExtension(ext)
if (!extensionModule) continue;
const LensExtensionClass: LensExtensionConstructor = extensionModule.default;
instance = new LensExtensionClass(ext);
instance.whenEnabled(() => register(instance));
this.instances.set(ext.manifestPath, instance);
instance.enable();
this.instances.set(extId, instance);
} catch (err) {
logger.error(`[EXTENSION-LOADER]: activation extension error`, { ext, err })
}
} else if (!ext.enabled && instance) {
try {
instance.disable();
this.instances.delete(extId);
} catch (err) {
logger.error(`[EXTENSIONS-LOADER]: init extension instance error`, { ext, err })
logger.error(`[EXTENSION-LOADER]: deactivation extension error`, { ext, err })
}
}
}
Expand All @@ -111,14 +139,21 @@ export class ExtensionLoader {
}
}

toJSON() {
return toJS(this.extensions, {
exportMapsAsObjects: false,
recurseEverything: true,
})
}

async broadcastExtensions(frameId?: number) {
await when(() => this.isLoaded);
broadcastIpc({
channel: "extensions:loaded",
frameId: frameId,
frameOnly: !!frameId,
args: [
Array.from(this.extensions.toJS().values())
Array.from(this.toJSON().values()),
],
})
}
Expand Down
13 changes: 7 additions & 6 deletions src/extensions/extension-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ import { extensionPackagesRoot } from "./extension-loader"
import { getBundledExtensions } from "../common/utils/app-version"

export interface InstalledExtension {
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this be a class? That way there isn't an abstraction boundary being broken by the loader.

manifest: LensExtensionManifest;
manifestPath: string;
isBundled?: boolean; // defined in package.json
readonly manifest: LensExtensionManifest;
readonly manifestPath: string;
readonly isBundled?: boolean; // defined in package.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even if you leave this as an interface, this should not be optional. Also, which package.json. I know it means Lens's but it could mean an extension's. Please update the comment.

enabled?: boolean;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I want to comment on Line 25 and maybe this should be done in a follow up PR (make that a certainly as to not hold up this PR too much) but I am very confused about ExtensionManager since it doesn't actually hold on to a list of extensions that are loaded.

type Dependencies = {
Expand Down Expand Up @@ -77,7 +78,7 @@ export class ExtensionManager {
return await this.loadExtensions();
}

protected async getByManifest(manifestPath: string): Promise<InstalledExtension> {
protected async getByManifest(manifestPath: string, { isBundled = false } = {}): Promise<InstalledExtension> {
let manifestJson: LensExtensionManifest;
try {
fs.accessSync(manifestPath, fs.constants.F_OK); // check manifest file for existence
Expand All @@ -88,6 +89,7 @@ export class ExtensionManager {
return {
manifestPath: path.join(this.nodeModulesPath, manifestJson.name, "package.json"),
manifest: manifestJson,
isBundled: isBundled,
}
} catch (err) {
logger.error(`[EXTENSION-MANAGER]: can't install extension at ${manifestPath}: ${err}`, { manifestJson });
Expand Down Expand Up @@ -129,9 +131,8 @@ export class ExtensionManager {
}
const absPath = path.resolve(folderPath, fileName);
const manifestPath = path.resolve(absPath, "package.json");
const ext = await this.getByManifest(manifestPath).catch(() => null)
const ext = await this.getByManifest(manifestPath, { isBundled: true }).catch(() => null)
if (ext) {
ext.isBundled = true;
extensions.push(ext)
}
}
Expand Down
49 changes: 49 additions & 0 deletions src/extensions/extensions-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import type { LensExtensionId } from "./lens-extension";
import { BaseStore } from "../common/base-store"
import { action, observable, toJS } from "mobx";

export interface LensExtensionsStoreModel {
extensions: Record<LensExtensionId, LensExtensionState>;
}

export interface LensExtensionState {
enabled?: boolean;
}

export class ExtensionsStore extends BaseStore<LensExtensionsStoreModel> {
constructor() {
super({
configName: "lens-extensions"
});
}

@observable extensions = observable.map<LensExtensionId, LensExtensionState>();

@action
setEnabled(extId: LensExtensionId, enabled: boolean) {
const state = this.extensions.get(extId);
this.extensions.set(extId, {
...(state || {}),
enabled: enabled,
})
}

isEnabled(extensionId: LensExtensionId) {
const state = this.extensions.get(extensionId);
return !state /* enabled by default */ || state.enabled;
}

protected fromStore({ extensions }: LensExtensionsStoreModel) {
this.extensions.merge(extensions);
}

toJSON(): LensExtensionsStoreModel {
return toJS({
extensions: this.extensions.toJSON(),
}, {
recurseEverything: true
})
}
}

export const extensionsStore = new ExtensionsStore();
41 changes: 6 additions & 35 deletions src/extensions/lens-extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import type { InstalledExtension } from "./extension-manager";
import { action, reaction } from "mobx";
import { action, observable, reaction } from "mobx";
import logger from "../main/logger";
import { ExtensionStore } from "./extension-store";

export type LensExtensionId = string; // path to manifest (package.json)
export type LensExtensionConstructor = new (...args: ConstructorParameters<typeof LensExtension>) => LensExtension;
Expand All @@ -14,35 +13,17 @@ export interface LensExtensionManifest {
renderer?: string; // path to %ext/dist/renderer.js
}

export interface LensExtensionStoreModel {
isEnabled: boolean;
}

export class LensExtension<S extends ExtensionStore<LensExtensionStoreModel> = any> {
protected store: S;
export class LensExtension {
readonly manifest: LensExtensionManifest;
readonly manifestPath: string;
readonly isBundled: boolean;

@observable private isEnabled = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

With this being private, I don't think it implements the InstalledExtension interface anymore, is that desirable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why you think it should implement? Using private field was suggested by @jakolehm

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe I don't have a grasp of how these types interact, I would have assumed that the extension-manager.ts file keeps a record to instances of this type. Does it not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope.. extension-loader.ts keeps both InstalledExtension list and enabled instances of those.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO that seems very strange, because we are keeping two lists of information that should always be the same. Why not just one 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.

Extension-manager doesn't keep anything.. Its purpose for install & load extensions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't extension-loader for installing and loading extensions? I would have assumed that extension-manager is the manager of truth and then uses loader to load stuff and store to persist data between restarts...

Copy link
Contributor

Choose a reason for hiding this comment

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

About private: without private extension instance itself cold toggle this which does not make any sense to me and is not really desirable. Why would we allow extension to do this?

ExtensionLoader: it's only purpose is to load (read: execute) extension instances. List of extensions are given to this class, it does not care how that list has generated.

ExtensionManager: it's only purpose is to gather a list of extensions and then install those to central node_modules folder (including all dependencies). Note: this does not load (or create) extension instances.

Naming things is one of the most difficult things in computer science so we probably could have better names for these. Still I strongly believe that separating complex things to smaller units is better than having one giant class that tries to do everything.


constructor({ manifest, manifestPath, isBundled }: InstalledExtension) {
this.manifest = manifest
this.manifestPath = manifestPath
this.isBundled = !!isBundled
this.init();
}

protected async init(store: S = createBaseStore().getInstance()) {
this.store = store;
await this.store.loadExtension(this);
reaction(() => this.store.data.isEnabled, (isEnabled = true) => {
this.toggle(isEnabled); // handle activation & deactivation
}, {
fireImmediately: true
});
}

get isEnabled() {
return !!this.store.data.isEnabled;
}

get id(): LensExtensionId {
Expand All @@ -64,15 +45,15 @@ export class LensExtension<S extends ExtensionStore<LensExtensionStoreModel> = a
@action
async enable() {
if (this.isEnabled) return;
this.store.data.isEnabled = true;
this.isEnabled = true;
this.onActivate();
logger.info(`[EXTENSION]: enabled ${this.name}@${this.version}`);
}

@action
async disable() {
if (!this.isEnabled) return;
this.store.data.isEnabled = false;
this.isEnabled = false;
this.onDeactivate();
logger.info(`[EXTENSION]: disabled ${this.name}@${this.version}`);
}
Expand Down Expand Up @@ -114,13 +95,3 @@ export class LensExtension<S extends ExtensionStore<LensExtensionStoreModel> = a
// mock
}
}

function createBaseStore() {
return class extends ExtensionStore<LensExtensionStoreModel> {
constructor() {
super({
configName: "state"
});
}
}
}
5 changes: 4 additions & 1 deletion src/main/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import { userStore } from "../common/user-store";
import { workspaceStore } from "../common/workspace-store";
import { appEventBus } from "../common/event-bus"
import { extensionLoader } from "../extensions/extension-loader";
import { extensionManager } from "../extensions/extension-manager";
import { extensionsStore } from "../extensions/extensions-store";

const workingDir = path.join(app.getPath("appData"), appName);
let proxyPort: number;
Expand Down Expand Up @@ -52,6 +54,7 @@ app.on("ready", async () => {
userStore.load(),
clusterStore.load(),
workspaceStore.load(),
extensionsStore.load(),
]);

// find free port
Expand All @@ -76,7 +79,7 @@ app.on("ready", async () => {
}

LensExtensionsApi.windowManager = windowManager = new WindowManager(proxyPort);
extensionLoader.init(); // call after windowManager to see splash earlier
extensionLoader.init(await extensionManager.load()); // call after windowManager to see splash earlier

setTimeout(() => {
appEventBus.emit({ name: "app", action: "start" })
Expand Down
2 changes: 2 additions & 0 deletions src/renderer/bootstrap.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { i18nStore } from "./i18n";
import { themeStore } from "./theme.store";
import { App } from "./components/app";
import { LensApp } from "./lens-app";
import { extensionsStore } from "../extensions/extensions-store";

type AppComponent = React.ComponentType & {
init?(): Promise<void>;
Expand All @@ -34,6 +35,7 @@ export async function bootstrap(App: AppComponent) {
userStore.load(),
workspaceStore.load(),
clusterStore.load(),
extensionsStore.load(),
i18nStore.init(),
themeStore.init(),
]);
Expand Down
16 changes: 11 additions & 5 deletions src/renderer/components/+extensions/extensions.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,17 @@
--width: 100%;
--max-width: auto;

.extension {
--flex-gap: $padding / 3;
padding: $padding $padding * 2;
background: $colorVague;
border-radius: $radius;
.extension-list {
.extension {
--flex-gap: $padding / 3;
padding: $padding $padding * 2;
background: $colorVague;
border-radius: $radius;

&:not(:first-of-type) {
margin-top: $padding * 2;
}
}
}

.extensions-path {
Expand Down
Loading