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

Resolve extension enabled status when loading it #1485

Merged
merged 6 commits into from Nov 25, 2020
Merged
Show file tree
Hide file tree
Changes from 4 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
23 changes: 15 additions & 8 deletions src/extensions/extension-discovery.ts
Expand Up @@ -6,6 +6,7 @@ import path from "path";
import { getBundledExtensions } from "../common/utils/app-version";
import logger from "../main/logger";
import { extensionInstaller, PackageJson } from "./extension-installer";
import { extensionsStore } from "./extensions-store";
import type { LensExtensionId, LensExtensionManifest } from "./lens-extension";

export interface InstalledExtension {
Expand Down Expand Up @@ -91,7 +92,7 @@ export class ExtensionDiscovery {
init() {
this.watchExtensions();
}

/**
* Watches for added/removed local extensions.
* Dependencies are installed automatically after an extension folder is copied.
Expand Down Expand Up @@ -213,24 +214,31 @@ export class ExtensionDiscovery {
}
}

protected async getByManifest(manifestPath: string, { isBundled = false, isEnabled = isBundled }: {
protected async getByManifest(manifestPath: string, { isBundled = false }: {
isBundled?: boolean;
isEnabled?: boolean;
} = {}): Promise<InstalledExtension | null> {
let manifestJson: LensExtensionManifest;
let isEnabled: boolean;

try {
// check manifest file for existence
fs.accessSync(manifestPath, fs.constants.F_OK);

manifestJson = __non_webpack_require__(manifestPath);
const installedManifestPath = path.join(this.nodeModulesPath, manifestJson.name, "package.json");
this.packagesJson.dependencies[manifestJson.name] = path.dirname(manifestPath);

if (!isBundled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] mutation could be avoided: const isEnabled = isBundled || extensionsStore.isEnabled(installedManifestPath);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 will change

isEnabled = extensionsStore.isEnabled(installedManifestPath);
} else {
isEnabled = true;
}

return {
manifestPath: path.join(this.nodeModulesPath, manifestJson.name, "package.json"),
manifestPath: installedManifestPath,
manifest: manifestJson,
isBundled,
isEnabled,
isEnabled
};
} catch (error) {
logger.error(`${logModule}: can't install extension at ${manifestPath}: ${error}`, { manifestJson });
Expand Down Expand Up @@ -316,13 +324,12 @@ export class ExtensionDiscovery {
/**
* Loads extension from absolute path, updates this.packagesJson to include it and returns the extension.
*/
async loadExtensionFromPath(absPath: string, { isBundled = false, isEnabled = isBundled }: {
async loadExtensionFromPath(absPath: string, { isBundled = false }: {
isBundled?: boolean;
isEnabled?: boolean;
} = {}): Promise<InstalledExtension | null> {
const manifestPath = path.resolve(absPath, manifestFilename);

return this.getByManifest(manifestPath, { isBundled, isEnabled });
return this.getByManifest(manifestPath, { isBundled });
}
}

Expand Down
5 changes: 0 additions & 5 deletions src/extensions/extensions-store.ts
Expand Up @@ -47,11 +47,6 @@ export class ExtensionsStore extends BaseStore<LensExtensionsStoreModel> {
await extensionLoader.whenLoaded;
await this.whenLoaded;

// activate user-extensions when state is ready
Copy link
Contributor Author

Choose a reason for hiding this comment

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

extensionManager has not loaded extensions at this point so we cannot determine the status here.

Copy link
Member

Choose a reason for hiding this comment

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

Should we add above something like await extensionManager.whenReady?

extensionLoader.userExtensions.forEach((ext, extId) => {
ext.isEnabled = this.isEnabled(extId);
});

// apply state on changes from store
reaction(() => this.state.toJS(), extensionsState => {
extensionsState.forEach((state, extId) => {
Expand Down