Mobile: Fix plugins not reloaded when the plugin runner reloads (#10540)

.eslintignore

@@ -676,7 +676,9 @@ packages/app-mobile/plugins/PluginRunner/dialogs/hooks/useWebViewSetup.js
 packages/app-mobile/plugins/PluginRunner/types.js
 packages/app-mobile/plugins/PluginRunner/utils/createOnLogHandler.js
 packages/app-mobile/plugins/hooks/usePlugin.js
+packages/app-mobile/plugins/loadPlugins.test.js
 packages/app-mobile/plugins/loadPlugins.js
+packages/app-mobile/plugins/testing/MockPluginRunner.js
 packages/app-mobile/root.js
 packages/app-mobile/services/AlarmServiceDriver.android.js
 packages/app-mobile/services/AlarmServiceDriver.ios.js

.gitignore

@@ -655,7 +655,9 @@ packages/app-mobile/plugins/PluginRunner/dialogs/hooks/useWebViewSetup.js
 packages/app-mobile/plugins/PluginRunner/types.js
 packages/app-mobile/plugins/PluginRunner/utils/createOnLogHandler.js
 packages/app-mobile/plugins/hooks/usePlugin.js
+packages/app-mobile/plugins/loadPlugins.test.js
 packages/app-mobile/plugins/loadPlugins.js
+packages/app-mobile/plugins/testing/MockPluginRunner.js
 packages/app-mobile/root.js
 packages/app-mobile/services/AlarmServiceDriver.android.js
 packages/app-mobile/services/AlarmServiceDriver.ios.js

packages/app-mobile/jest.setup.js

@@ -29,6 +29,16 @@ jest.mock('react-native-device-info', () => {
 	};
 });
 
+// react-native-version-info doesn't work (returns undefined for .version) when
+// running in a testing environment.
+jest.doMock('react-native-version-info', () => {
+	return {
+		default: {
+			appVersion: require('./package.json').version,
+		},
+	};
+});
+
 // react-native-webview expects native iOS/Android code so needs to be mocked.
 jest.mock('react-native-webview', () => {
 	const { View } = require('react-native');
@@ -37,6 +47,10 @@ jest.mock('react-native-webview', () => {
 	};
 });
 
+jest.mock('@react-native-clipboard/clipboard', () => {
+	return { default: { getString: jest.fn(), setString: jest.fn() } };
+});
+
 // react-native-fs's CachesDirectoryPath export doesn't work in a testing environment.
 // Use a temporary folder instead.
 const tempDirectoryPath = path.join(tmpdir(), `appmobile-test-${uuid.createNano()}`);

packages/app-mobile/plugins/PluginRunner/PluginRunnerWebView.tsx

@@ -13,6 +13,7 @@ import { PluginHtmlContents, PluginStates } from '@joplin/lib/services/plugins/r
 import useAsyncEffect from '@joplin/lib/hooks/useAsyncEffect';
 import PluginDialogManager from './dialogs/PluginDialogManager';
 import { AppState } from '../../utils/types';
+import usePrevious from '@joplin/lib/hooks/usePrevious';
 
 const logger = Logger.create('PluginRunnerWebView');
 
@@ -28,14 +29,25 @@ const usePlugins = (
 	webviewLoaded: boolean,
 	pluginSettings: PluginSettings,
 ) => {
-	const store = useStore();
+	const store = useStore<AppState>();
+	const lastPluginRunner = usePrevious(pluginRunner);
+
+	// Only set reloadAll to true here -- this ensures that all plugins are reloaded,
+	// even if loadPlugins is cancelled and re-run.
+	const reloadAllRef = useRef(false);
+	reloadAllRef.current ||= pluginRunner !== lastPluginRunner;
 
 	useAsyncEffect(async (event) => {
 		if (!webviewLoaded) {
 			return;
 		}
 
-		void loadPlugins(pluginRunner, pluginSettings, store, event);
+		await loadPlugins({ pluginRunner, pluginSettings, store, reloadAll: reloadAllRef.current, cancelEvent: event });
+
+		// A full reload, if it was necessary, has been completed.
+		if (!event.cancelled) {
+			reloadAllRef.current = false;
+		}
 	}, [pluginRunner, store, webviewLoaded, pluginSettings]);
 };
 
@@ -134,7 +146,7 @@ const PluginRunnerWebViewComponent: React.FC<Props> = props => {
 		return (
 			<>
 				<ExtendedWebView
-					webviewInstanceId='PluginRunner'
+					webviewInstanceId='PluginRunner2'
 					html={html}
 					injectedJavaScript={injectedJs}
 					hasPluginScripts={true}

packages/app-mobile/plugins/loadPlugins.test.ts

@@ -0,0 +1,148 @@
+import Setting from '@joplin/lib/models/Setting';
+import PluginService, { defaultPluginSetting } from '@joplin/lib/services/plugins/PluginService';
+import { PluginManifest } from '@joplin/lib/services/plugins/utils/types';
+import { setupDatabaseAndSynchronizer, switchClient } from '@joplin/lib/testing/test-utils';
+import { writeFile } from 'fs-extra';
+import { join } from 'path';
+import loadPlugins, { Props as LoadPluginsProps } from './loadPlugins';
+import createMockReduxStore from '../utils/testing/createMockReduxStore';
+import MockPluginRunner from './testing/MockPluginRunner';
+
+const setPluginEnabled = (id: string, enabled: boolean) => {
+	const newPluginStates = {
+		...Setting.value('plugins.states'),
+		[id]: {
+			...defaultPluginSetting(),
+			enabled,
+		},
+	};
+	Setting.setValue('plugins.states', newPluginStates);
+};
+
+const addPluginWithManifest = async (manifest: PluginManifest, enabled: boolean) => {
+	const pluginSource = `
+		/* joplin-manifest:
+		${JSON.stringify(manifest)}
+		*/
+
+		joplin.plugins.register({
+			onStart: async function() { },
+		});
+	`;
+	const pluginPath = join(Setting.value('pluginDir'), `${manifest.id}.js`);
+	await writeFile(pluginPath, pluginSource, 'utf-8');
+
+	setPluginEnabled(manifest.id, enabled);
+};
+
+const defaultManifestProperties = {
+	manifest_version: 1,
+	version: '0.1.0',
+	app_min_version: '2.3.4',
+	platforms: ['desktop', 'mobile'],
+};
+
+describe('loadPlugins', () => {
+	beforeEach(async () => {
+		await setupDatabaseAndSynchronizer(1);
+		await switchClient(1);
+	});
+
+	afterEach(async () => {
+		for (const id of PluginService.instance().pluginIds) {
+			await PluginService.instance().unloadPlugin(id);
+		}
+		await PluginService.instance().destroy();
+	});
+
+	test('should load only enabled plugins', async () => {
+		await addPluginWithManifest({
+			...defaultManifestProperties,
+			id: 'this.is.a.test.1',
+			name: 'Disabled Plugin',
+		}, false);
+
+		const enabledPluginId = 'this.is.a.test.2';
+		await addPluginWithManifest({
+			...defaultManifestProperties,
+			id: enabledPluginId,
+			name: 'Enabled Plugin',
+		}, true);
+
+		const pluginRunner = new MockPluginRunner();
+		const store = createMockReduxStore();
+
+		const loadPluginsOptions: LoadPluginsProps = {
+			pluginRunner,
+			pluginSettings: Setting.value('plugins.states'),
+			store,
+			reloadAll: false,
+			cancelEvent: { cancelled: false },
+		};
+		expect(Object.keys(PluginService.instance().plugins)).toHaveLength(0);
+
+		await loadPlugins(loadPluginsOptions);
+		await pluginRunner.waitForAllToBeRunning([enabledPluginId]);
+
+		expect(pluginRunner.runningPluginIds).toMatchObject([enabledPluginId]);
+		// No plugins were running before, so none should be stopped.
+		expect(pluginRunner.stopCalledTimes).toBe(0);
+
+		// Loading again should not re-run plugins
+		await loadPlugins(loadPluginsOptions);
+
+		// Should have tried to stop at most the disabled plugin (which is a no-op).
+		expect(pluginRunner.stopCalledTimes).toBe(1);
+		expect(pluginRunner.runningPluginIds).toMatchObject([enabledPluginId]);
+	});
+
+	test('should reload all plugins when reloadAll is true', async () => {
+		const enabledCount = 3;
+		for (let i = 0; i < enabledCount; i++) {
+			await addPluginWithManifest({
+				...defaultManifestProperties,
+				id: `joplin.test.plugin.${i}`,
+				name: `Enabled Plugin ${i}`,
+			}, true);
+		}
+
+		const disabledCount = 6;
+		for (let i = 0; i < disabledCount; i++) {
+			await addPluginWithManifest({
+				...defaultManifestProperties,
+				id: `joplin.test.plugin.disabled.${i}`,
+				name: `Disabled Plugin ${i}`,
+			}, false);
+		}
+
+		const pluginRunner = new MockPluginRunner();
+		const store = createMockReduxStore();
+		const loadPluginsOptions: LoadPluginsProps = {
+			pluginRunner,
+			pluginSettings: Setting.value('plugins.states'),
+			store,
+			reloadAll: true,
+			cancelEvent: { cancelled: false },
+		};
+		await loadPlugins(loadPluginsOptions);
+		let expectedRunningIds = ['joplin.test.plugin.0', 'joplin.test.plugin.1', 'joplin.test.plugin.2'];
+		await pluginRunner.waitForAllToBeRunning(expectedRunningIds);
+
+		// No additional plugins should be running.
+		expect([...pluginRunner.runningPluginIds].sort()).toMatchObject(expectedRunningIds);
+
+		// No plugins were running before -- there were no plugins to stop
+		expect(pluginRunner.stopCalledTimes).toBe(0);
+
+		// Enabling a plugin and reloading it should cause all plugins to load.
+		setPluginEnabled('joplin.test.plugin.disabled.2', true);
+		await loadPlugins({ ...loadPluginsOptions, pluginSettings: Setting.value('plugins.states') });
+		expectedRunningIds = ['joplin.test.plugin.0', 'joplin.test.plugin.1', 'joplin.test.plugin.2', 'joplin.test.plugin.disabled.2'];
+		await pluginRunner.waitForAllToBeRunning(expectedRunningIds);
+
+		// Reloading all should stop all plugins and rerun enabled plugins, even
+		// if not enabled previously.
+		expect(pluginRunner.stopCalledTimes).toBe(disabledCount + enabledCount);
+		expect([...pluginRunner.runningPluginIds].sort()).toMatchObject(expectedRunningIds);
+	});
+});

packages/app-mobile/plugins/loadPlugins.ts

@@ -1,4 +1,3 @@
-
 import Setting from '@joplin/lib/models/Setting';
 import BasePluginRunner from '@joplin/lib/services/plugins/BasePluginRunner';
 import PluginService, { PluginSettings } from '@joplin/lib/services/plugins/PluginService';
@@ -6,15 +5,21 @@ import PlatformImplementation from './PlatformImplementation';
 import { Store } from 'redux';
 import Logger from '@joplin/utils/Logger';
 import shim from '@joplin/lib/shim';
+import { AppState } from '../utils/types';
 
 const logger = Logger.create('loadPlugins');
 
 type CancelEvent = { cancelled: boolean };
 
-const loadPlugins = async (
-	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
-	pluginRunner: BasePluginRunner, pluginSettings: PluginSettings, store: Store<any>, cancel: CancelEvent,
-) => {
+export interface Props {
+	pluginRunner: BasePluginRunner;
+	pluginSettings: PluginSettings;
+	store: Store<AppState>;
+	reloadAll: boolean;
+	cancelEvent: CancelEvent;
+}
+
+const loadPlugins = async ({ pluginRunner, pluginSettings, store, reloadAll, cancelEvent }: Props) => {
 	try {
 		const pluginService = PluginService.instance();
 		const platformImplementation = PlatformImplementation.instance();
@@ -23,13 +28,18 @@ const loadPlugins = async (
 		);
 		pluginService.isSafeMode = Setting.value('isSafeMode');
 
-		for (const pluginId of Object.keys(pluginService.plugins)) {
-			if (pluginSettings[pluginId] && !pluginSettings[pluginId].enabled) {
-				logger.info('Unloading disabled plugin', pluginId);
+		if (reloadAll) {
+			logger.info('Reloading all plugins.');
+		}
+
+		for (const pluginId of pluginService.pluginIds) {
+			if (reloadAll || (pluginSettings[pluginId] && !pluginSettings[pluginId].enabled)) {
+				logger.info('Unloading plugin', pluginId);
 				await pluginService.unloadPlugin(pluginId);
 			}
 
-			if (cancel.cancelled) {
+			if (cancelEvent.cancelled) {
+				logger.info('Cancelled.');
 				return;
 			}
 		}
@@ -39,7 +49,8 @@ const loadPlugins = async (
 			await pluginService.loadAndRunDevPlugins(pluginSettings);
 		}
 
-		if (cancel.cancelled) {
+		if (cancelEvent.cancelled) {
+			logger.info('Cancelled.');
 			return;
 		}
 

packages/app-mobile/plugins/testing/MockPluginRunner.ts

@@ -0,0 +1,47 @@
+import BasePluginRunner from '@joplin/lib/services/plugins/BasePluginRunner';
+import Plugin from '@joplin/lib/services/plugins/Plugin';
+
+
+export default class MockPluginRunner extends BasePluginRunner {
+	public runningPluginIds: string[] = [];
+	private onRunningPluginsChangedListeners_ = [() => {}];
+	private stopCalledTimes_ = 0;
+
+	public waitForAllToBeRunning(expectedIds: string[]) {
+		return new Promise<void>(resolve => {
+			const listener = () => {
+				let missingIds = false;
+				for (const id of expectedIds) {
+					if (!this.runningPluginIds.includes(id)) {
+						missingIds = true;
+						console.warn('Missing ID', id, 'in', this.runningPluginIds);
+						break;
+					}
+				}
+				if (!missingIds) {
+					this.onRunningPluginsChangedListeners_ = this.onRunningPluginsChangedListeners_.filter(l => l !== listener);
+					resolve();
+				}
+			};
+			this.onRunningPluginsChangedListeners_.push(listener);
+			listener();
+		});
+	}
+
+	public get stopCalledTimes() { return this.stopCalledTimes_; }
+
+	public override async run(plugin: Plugin) {
+		this.runningPluginIds.push(plugin.manifest.id);
+
+		for (const listener of this.onRunningPluginsChangedListeners_) {
+			listener();
+		}
+	}
+
+	public override async stop(plugin: Plugin) {
+		this.runningPluginIds = this.runningPluginIds.filter(id => id !== plugin.manifest.id);
+		this.stopCalledTimes_ ++;
+	}
+
+	public override async waitForSandboxCalls() {}
+}