add tests and logging

packages/lib/services/plugins/PluginService.ts

@@ -19,6 +19,11 @@ const uslug = require('@joplin/fork-uslug');
 
 const logger = Logger.create('PluginService');
 
+interface PluginExtractionState {
+	size: number;
+	timestamp: number;
+}
+
 // Plugin data is split into two:
 //
 // - First there's the service `plugins` property, which contains the
@@ -105,6 +110,7 @@ export default class PluginService extends BaseService {
 	private startedPlugins_: Record<string, boolean> = {};
 	private isSafeMode_ = false;
 	private pluginsChangeListeners_: LoadedPluginsChangeListener[] = [];
+	private extractionStates_: Record<string, PluginExtractionState> = null;
 
 	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
 	public initialize(appVersion: string, platformImplementation: any, runner: BasePluginRunner, store: any) {
@@ -197,6 +203,27 @@ export default class PluginService extends BaseService {
 		await shim.fsDriver().remove(plugin.baseDir);
 	}
 
+	private extractionStatePath(): string {
+		return `${Setting.value('cacheDir')}/plugin-extraction-state.json`;
+	}
+
+	private async loadExtractionStates(): Promise<Record<string, PluginExtractionState>> {
+		if (!this.extractionStates_) {
+			try {
+				const text = await shim.fsDriver().readFile(this.extractionStatePath(), 'utf8');
+				this.extractionStates_ = JSON.parse(text);
+			} catch {
+				this.extractionStates_ = {};
+			}
+		}
+		return this.extractionStates_;
+	}
+
+	private async saveExtractionStates(states: Record<string, PluginExtractionState>): Promise<void> {
+		this.extractionStates_ = states;
+		await shim.fsDriver().writeFile(this.extractionStatePath(), JSON.stringify(states), 'utf8');
+	}
+
 	public pluginById(id: string): Plugin {
 		if (!this.plugins_[id]) throw new Error(`Plugin not found: ${id}`);
 
@@ -287,29 +314,26 @@ export default class PluginService extends BaseService {
 		return this.loadPlugin(baseDir, r.manifestText, r.scriptText, pluginIdIfNotSpecified);
 	}
 
-	public async loadPluginFromPackage(baseDir: string, path: string, manifestOnly = false): Promise<Plugin> {
+	public async loadPluginFromPackage(baseDir: string, path: string): Promise<Plugin> {
 		baseDir = rtrimSlashes(baseDir);
 
 		const fname = filename(path);
 		const unpackDir = `${Setting.value('cacheDir')}/${fname}`;
 		const manifestFilePath = `${unpackDir}/manifest.json`;
 
-		if (manifestOnly) {
-			// When loading only the manifest (e.g. for disabled plugins), try
-			// to use an already-extracted manifest from cache to avoid the
-			// expensive MD5 hash and tar extraction.
-			const manifest = await this.loadManifestToObject(manifestFilePath);
-			if (manifest) {
-				return this.loadPlugin(unpackDir, JSON.stringify(manifest), '', makePluginId(fname));
-			}
-		}
+		// Use file size + mtime to check if the .jpl has changed, to
+		// avoid computing an MD5 hash of the full file on every startup.
+		const stat = await shim.fsDriver().stat(path);
+		const extractionStates = await this.loadExtractionStates();
+		const extractionState = extractionStates[fname];
+		const extractionValid = extractionState
+			&& extractionState.size === stat.size
+			&& extractionState.timestamp === stat.mtime.getTime()
+			&& await shim.fsDriver().exists(manifestFilePath);
 
-		const hash = await shim.fsDriver().md5File(path);
+		if (!extractionValid) {
+			logger.info(`Extracting plugin: ${fname}`);
 
-		// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
-		let manifest: any = await this.loadManifestToObject(manifestFilePath);
-
-		if (!manifest || manifest._package_hash !== hash) {
 			await shim.fsDriver().remove(unpackDir);
 			await shim.fsDriver().mkdir(unpackDir);
 
@@ -320,15 +344,19 @@ export default class PluginService extends BaseService {
 				cwd: unpackDir,
 			});
 
-			manifest = await this.loadManifestToObject(manifestFilePath);
+			const manifest = await this.loadManifestToObject(manifestFilePath);
 			if (!manifest) throw new Error(`Missing manifest file at: ${manifestFilePath}`);
 
-			manifest._package_hash = hash;
-
-			await shim.fsDriver().writeFile(manifestFilePath, JSON.stringify(manifest, null, '\t'), 'utf8');
+			extractionStates[fname] = {
+				size: stat.size,
+				timestamp: stat.mtime.getTime(),
+			};
+			await this.saveExtractionStates(extractionStates);
+		} else {
+			logger.info(`Using already extracted plugin: ${fname}`);
 		}
 
-		return this.loadPluginFromPath(unpackDir, manifestOnly);
+		return this.loadPluginFromPath(unpackDir);
 	}
 
 	// Loads the manifest as a simple object with no validation. Used only
@@ -343,7 +371,7 @@ export default class PluginService extends BaseService {
 		}
 	}
 
-	public async loadPluginFromPath(path: string, manifestOnly = false): Promise<Plugin> {
+	public async loadPluginFromPath(path: string): Promise<Plugin> {
 		path = rtrimSlashes(path);
 
 		const fsDriver = shim.fsDriver();
@@ -351,7 +379,7 @@ export default class PluginService extends BaseService {
 		if (path.toLowerCase().endsWith('.js')) {
 			return this.loadPluginFromJsBundle(dirname(path), await fsDriver.readFile(path), filename(path));
 		} else if (path.toLowerCase().endsWith('.jpl')) {
-			return this.loadPluginFromPackage(dirname(path), path, manifestOnly);
+			return this.loadPluginFromPackage(dirname(path), path);
 		} else {
 			let distPath = path;
 			if (!(await fsDriver.exists(`${distPath}/manifest.json`))) {
@@ -360,8 +388,8 @@ export default class PluginService extends BaseService {
 
 			logger.info(`Loading plugin from ${path}`);
 
+			const scriptText = await fsDriver.readFile(`${distPath}/index.js`);
 			const manifestText = await fsDriver.readFile(`${distPath}/manifest.json`);
-			const scriptText = manifestOnly ? '' : await fsDriver.readFile(`${distPath}/index.js`);
 			const pluginId = makePluginId(filename(path));
 
 			return this.loadPlugin(distPath, manifestText, scriptText, pluginId);
@@ -459,16 +487,8 @@ export default class PluginService extends BaseService {
 			}
 
 			try {
-				// Load only the manifest first to check if the plugin is
-				// enabled before doing the expensive full load.
-				let plugin = await this.loadPluginFromPath(pluginPath, true);
+				const plugin = await this.loadPluginFromPath(pluginPath);
 				const enabled = this.pluginEnabled(settings, plugin.id);
-				if (enabled) {
-					logger.info(`Loading full plugin: ${plugin.id}`);
-					plugin = await this.loadPluginFromPath(pluginPath, false);
-				} else {
-					logger.info(`Loading manifest only for disabled plugin: ${plugin.id}`);
-				}
 
 				const existingPlugin = this.plugins_[plugin.id];
 				if (existingPlugin) {
@@ -513,6 +533,7 @@ export default class PluginService extends BaseService {
 				logger.error(`Could not load plugin: ${pluginPath}`, error);
 			}
 		}
+
 	}
 
 	public async loadAndRunDevPlugins(settings: PluginSettings) {

packages/lib/services/plugins/loadPlugins.test.ts

@@ -1,5 +1,5 @@
 import Setting from '../../models/Setting';
-import PluginService, { defaultPluginSetting } from '../../services/plugins/PluginService';
+import PluginService from '../../services/plugins/PluginService';
 import { setupDatabaseAndSynchronizer, switchClient, withWarningSilenced } from '../../testing/test-utils';
 import loadPlugins, { Props as LoadPluginsProps } from './loadPlugins';
 import MockPluginRunner from './testing/MockPluginRunner';
@@ -8,9 +8,7 @@ import { Action, createStore } from 'redux';
 import MockPlatformImplementation from './testing/MockPlatformImplementation';
 import createTestPlugin from '../../testing/plugins/createTestPlugin';
 import Plugin from './Plugin';
-import { PluginManifest } from './utils/types';
-import { writeFile, mkdirp } from 'fs-extra';
-import { join } from 'path';
+import shim from '../../shim';
 
 const createMockReduxStore = () => {
 	return createStore((state: State = defaultState, action: Action<string>) => {
@@ -139,31 +137,42 @@ describe('loadPlugins', () => {
 		expect([...pluginRunner.runningPluginIds].sort()).toMatchObject(expectedRunningIds);
 	});
 
-	test('should not load the script for disabled plugins', async () => {
-		const createDirPlugin = async (manifest: PluginManifest, enabled: boolean) => {
-			const dir = join(Setting.value('pluginDir'), manifest.id);
-			await mkdirp(dir);
-			await writeFile(join(dir, 'manifest.json'), JSON.stringify(manifest), 'utf-8');
-			await writeFile(join(dir, 'index.js'), 'joplin.plugins.register({ onStart: async function() {} });', 'utf-8');
-			const newStates = {
-				...Setting.value('plugins.states'),
-				[manifest.id]: { ...defaultPluginSetting(), enabled },
-			};
-			Setting.setValue('plugins.states', newStates);
-		};
-
-		const enabledId = 'joplin.test.plugin.enabled';
-		const disabledId = 'joplin.test.plugin.disabled';
-		await createDirPlugin({ ...defaultManifestProperties, id: enabledId, name: 'Enabled' }, true);
-		await createDirPlugin({ ...defaultManifestProperties, id: disabledId, name: 'Disabled' }, false);
+	test('should skip extraction when jpl has not changed', async () => {
+		const pluginId = 'joplin.test.plugin.packed';
+		await createTestPlugin({
+			...defaultManifestProperties,
+			id: pluginId,
+			name: 'Test JPL Plugin',
+		}, { format: 'jpl' });
 
 		const pluginRunner = new MockPluginRunner();
 		const store = createMockReduxStore();
-		PluginService.instance().initialize('2.3.4', platformImplementation, pluginRunner, store);
-		await PluginService.instance().loadAndRunPlugins(Setting.value('pluginDir'), Setting.value('plugins.states'));
+		const service = PluginService.instance();
+		service.initialize('2.3.4', platformImplementation, pluginRunner, store);
 
-		expect(PluginService.instance().plugins[disabledId].scriptText).toBe('');
-		expect(PluginService.instance().plugins[enabledId].scriptText).not.toBe('');
+		const tarExtractSpy = jest.spyOn(shim.fsDriver(), 'tarExtract');
+
+		// First load should extract
+		await service.loadAndRunPlugins(Setting.value('pluginDir'), Setting.value('plugins.states'));
+		expect(tarExtractSpy).toHaveBeenCalledTimes(1);
+
+		// Second load with same file should skip extraction
+		await service.unloadPlugin(pluginId);
+		await service.loadAndRunPlugins(Setting.value('pluginDir'), Setting.value('plugins.states'));
+		expect(tarExtractSpy).toHaveBeenCalledTimes(1);
+
+		// Recreating the jpl (different mtime/size) should trigger re-extraction
+		await service.unloadPlugin(pluginId);
+		await createTestPlugin({
+			...defaultManifestProperties,
+			id: pluginId,
+			name: 'Test JPL Plugin',
+		}, { format: 'jpl', onStart: '/* changed */' });
+
+		await service.loadAndRunPlugins(Setting.value('pluginDir'), Setting.value('plugins.states'));
+		expect(tarExtractSpy).toHaveBeenCalledTimes(2);
+
+		tarExtractSpy.mockRestore();
 	});
 
 	test('should not block allPluginsStarted when a plugin fails to start', async () => {

packages/lib/testing/plugins/createTestPlugin.ts

@@ -1,8 +1,9 @@
 import { join } from 'path';
 import { PluginManifest } from '../../services/plugins/utils/types';
 import Setting from '../../models/Setting';
-import { writeFile } from 'fs-extra';
+import { mkdirp, writeFile } from 'fs-extra';
 import { defaultPluginSetting } from '../../services/plugins/PluginService';
+import shim from '../../shim';
 
 
 const setPluginEnabled = (id: string, enabled: boolean) => {
@@ -19,22 +20,35 @@ const setPluginEnabled = (id: string, enabled: boolean) => {
 interface Options {
 	onStart?: string;
 	enabled?: boolean;
+	format?: 'js' | 'jpl';
 }
 
-const createTestPlugin = async (manifest: PluginManifest, { onStart = '', enabled = true }: Options = {}) => {
-	const pluginSource = `
+const createTestPlugin = async (manifest: PluginManifest, { onStart = '', enabled = true, format = 'js' }: Options = {}) => {
+	const scriptSource = `joplin.plugins.register({
+		onStart: async function() {
+			${onStart}
+		},
+	});`;
+
+	if (format === 'jpl') {
+		const tempDir = join(Setting.value('tempDir'), `plugin-build-${manifest.id}`);
+		await mkdirp(tempDir);
+		await writeFile(join(tempDir, 'manifest.json'), JSON.stringify(manifest), 'utf-8');
+		await writeFile(join(tempDir, 'index.js'), scriptSource, 'utf-8');
+
+		const jplPath = join(Setting.value('pluginDir'), `${manifest.id}.jpl`);
+		await shim.fsDriver().tarCreate({ cwd: tempDir, file: jplPath }, ['manifest.json', 'index.js']);
+	} else {
+		const pluginSource = `
 		/* joplin-manifest:
 		${JSON.stringify(manifest)}
 		*/
 
-		joplin.plugins.register({
-			onStart: async function() {
-				${onStart}
-			},
-		});
+		${scriptSource}
 	`;
-	const pluginPath = join(Setting.value('pluginDir'), `${manifest.id}.js`);
-	await writeFile(pluginPath, pluginSource, 'utf-8');
+		const pluginPath = join(Setting.value('pluginDir'), `${manifest.id}.js`);
+		await writeFile(pluginPath, pluginSource, 'utf-8');
+	}
 
 	setPluginEnabled(manifest.id, enabled);