feat: Use dynamic import for external JS (#26522)

Author: Nerivec

lib/extension/externalConverters.ts

@@ -1,13 +1,13 @@
-import type * as zhc from 'zigbee-herdsman-converters';
+import type {ExternalDefinitionWithExtend} from 'zigbee-herdsman-converters';
 
 import {addExternalDefinition, removeExternalDefinitions} from 'zigbee-herdsman-converters';
 
 import logger from '../util/logger';
 import ExternalJSExtension from './externalJS';
 
-type ModuleExports = zhc.ExternalDefinitionWithExtend | zhc.ExternalDefinitionWithExtend[];
+type TModule = ExternalDefinitionWithExtend | ExternalDefinitionWithExtend[];
 
-export default class ExternalConverters extends ExternalJSExtension<ModuleExports> {
+export default class ExternalConverters extends ExternalJSExtension<TModule> {
     constructor(
         zigbee: Zigbee,
         mqtt: MQTT,
@@ -33,29 +33,32 @@ export default class ExternalConverters extends ExternalJSExtension<ModuleExport
     }
 
     // eslint-disable-next-line @typescript-eslint/no-unused-vars
-    protected async removeJS(name: string, module: ModuleExports): Promise<void> {
+    protected async removeJS(name: string, mod: TModule): Promise<void> {
         removeExternalDefinitions(name);
 
         await this.zigbee.resolveDevicesDefinitions(true);
     }
 
-    protected async loadJS(name: string, module: ModuleExports): Promise<void> {
+    protected async loadJS(name: string, mod: TModule, newName?: string): Promise<void> {
         try {
             removeExternalDefinitions(name);
 
-            const definitions = Array.isArray(module) ? module : [module];
+            const definitions = Array.isArray(mod) ? mod : [mod];
+
             for (const definition of definitions) {
-                definition.externalConverterName = name;
+                definition.externalConverterName = newName ?? name;
 
                 addExternalDefinition(definition);
-                logger.info(`Loaded external converter '${name}'.`);
+                logger.info(`Loaded external converter '${newName ?? name}'.`);
             }
 
             await this.zigbee.resolveDevicesDefinitions(true);
         } catch (error) {
-            logger.error(`Failed to load external converter '${name}'`);
-            logger.error(`Check the code for syntax error and make sure it is up to date with the current Zigbee2MQTT version.`);
             logger.error(
+                /* v8 ignore next */
+                `Failed to load external converter '${newName ?? name}'. Check the code for syntax error and make sure it is up to date with the current Zigbee2MQTT version.`,
+            );
+            logger.warning(
                 `External converters are not meant for long term usage, but for local testing after which a pull request should be created to add out-of-the-box support for the device`,
             );
 

lib/extension/externalExtensions.ts

@@ -4,9 +4,9 @@ import logger from '../util/logger';
 import * as settings from '../util/settings';
 import ExternalJSExtension from './externalJS';
 
-type ModuleExports = typeof Extension;
+type TModule = new (...args: ConstructorParameters<typeof Extension>) => Extension;
 
-export default class ExternalExtensions extends ExternalJSExtension<ModuleExports> {
+export default class ExternalExtensions extends ExternalJSExtension<TModule> {
     constructor(
         zigbee: Zigbee,
         mqtt: MQTT,
@@ -31,29 +31,40 @@ export default class ExternalExtensions extends ExternalJSExtension<ModuleExport
         );
     }
 
-    protected async removeJS(name: string, module: ModuleExports): Promise<void> {
-        await this.enableDisableExtension(false, module.name);
+    protected async removeJS(name: string, mod: TModule): Promise<void> {
+        await this.enableDisableExtension(false, mod.name);
     }
 
-    protected async loadJS(name: string, module: ModuleExports): Promise<void> {
-        // stop if already started
-        await this.enableDisableExtension(false, module.name);
-        await this.addExtension(
-            // @ts-expect-error `module` is the interface, not the actual passed class
-            new module(
-                this.zigbee,
-                this.mqtt,
-                this.state,
-                this.publishEntityState,
-                this.eventBus,
-                this.enableDisableExtension,
-                this.restartCallback,
-                this.addExtension,
-                settings,
-                logger,
-            ),
-        );
+    protected async loadJS(name: string, mod: TModule, newName?: string): Promise<void> {
+        try {
+            // stop if already started
+            await this.enableDisableExtension(false, mod.name);
+            await this.addExtension(
+                new mod(
+                    this.zigbee,
+                    this.mqtt,
+                    this.state,
+                    this.publishEntityState,
+                    this.eventBus,
+                    this.enableDisableExtension,
+                    this.restartCallback,
+                    this.addExtension,
+                    // @ts-expect-error additional params that don't fit the internal `Extension` type
+                    settings,
+                    logger,
+                ),
+            );
+
+            /* v8 ignore start */
+            logger.info(`Loaded external extension '${newName ?? name}'.`);
+        } catch (error) {
+            logger.error(
+                /* v8 ignore next */
+                `Failed to load external extension '${newName ?? name}'. Check the code for syntax error and make sure it is up to date with the current Zigbee2MQTT version.`,
+            );
 
-        logger.info(`Loaded external extension '${name}'.`);
+            throw error;
+        }
+        /* v8 ignore stop */
     }
 }

lib/extension/externalJS.ts

@@ -2,7 +2,6 @@ import type {Zigbee2MQTTAPI, Zigbee2MQTTResponse} from '../types/api';
 
 import fs from 'node:fs';
 import path from 'node:path';
-import {Context, runInNewContext} from 'node:vm';
 
 import bind from 'bind-decorator';
 import stringify from 'json-stable-stringify-without-jsonify';
@@ -16,9 +15,11 @@ import Extension from './extension';
 const SUPPORTED_OPERATIONS = ['save', 'remove'];
 
 export default abstract class ExternalJSExtension<M> extends Extension {
+    protected folderName: string;
     protected mqttTopic: string;
     protected requestRegex: RegExp;
     protected basePath: string;
+    protected srcBasePath: string;
 
     constructor(
         zigbee: Zigbee,
@@ -34,9 +35,17 @@ export default abstract class ExternalJSExtension<M> extends Extension {
     ) {
         super(zigbee, mqtt, state, publishEntityState, eventBus, enableDisableExtension, restartCallback, addExtension);
 
+        this.folderName = folderName;
         this.mqttTopic = mqttTopic;
         this.requestRegex = new RegExp(`${settings.get().mqtt.base_topic}/bridge/request/${mqttTopic}/(save|remove)`);
         this.basePath = data.joinPath(folderName);
+        // 1-up from this file
+        this.srcBasePath = path.join(
+            __dirname,
+            '..',
+            // prevent race in vitest with files being manipulated from same location
+            process.env.VITEST_WORKER_ID ? /* v8 ignore next */ `${folderName}_${Math.floor(Math.random() * 10000)}` : folderName,
+        );
     }
 
     override async start(): Promise<void> {
@@ -46,25 +55,35 @@ export default abstract class ExternalJSExtension<M> extends Extension {
         await this.publishExternalJS();
     }
 
-    private getFilePath(name: string, mkBasePath: boolean = false): string {
-        if (mkBasePath && !fs.existsSync(this.basePath)) {
-            fs.mkdirSync(this.basePath, {recursive: true});
+    override async stop(): Promise<void> {
+        // remove src base path on stop to ensure always back to default
+        fs.rmSync(this.srcBasePath, {force: true, recursive: true});
+        await super.stop();
+    }
+
+    private getFilePath(name: string, mkBasePath = false, inSource = false): string {
+        const basePath = inSource ? this.srcBasePath : this.basePath;
+
+        if (mkBasePath && !fs.existsSync(basePath)) {
+            fs.mkdirSync(basePath, {recursive: true});
         }
 
-        return path.join(this.basePath, name);
+        return path.join(basePath, name);
     }
 
     protected getFileCode(name: string): string {
-        return fs.readFileSync(path.join(this.basePath, name), 'utf8');
+        return fs.readFileSync(this.getFilePath(name), 'utf8');
     }
 
-    protected *getFiles(): Generator<{name: string; code: string}> {
-        if (!fs.existsSync(this.basePath)) {
+    protected *getFiles(inSource = false): Generator<{name: string; code: string}> {
+        const basePath = inSource ? this.srcBasePath : this.basePath;
+
+        if (!fs.existsSync(basePath)) {
             return;
         }
 
-        for (const fileName of fs.readdirSync(this.basePath)) {
-            if (fileName.endsWith('.js')) {
+        for (const fileName of fs.readdirSync(basePath)) {
+            if (fileName.endsWith('.js') || fileName.endsWith('.cjs') || fileName.endsWith('.mjs')) {
                 yield {name: fileName, code: this.getFileCode(fileName)};
             }
         }
@@ -100,9 +119,9 @@ export default abstract class ExternalJSExtension<M> extends Extension {
         }
     }
 
-    protected abstract removeJS(name: string, module: M): Promise<void>;
+    protected abstract removeJS(name: string, mod: M): Promise<void>;
 
-    protected abstract loadJS(name: string, module: M): Promise<void>;
+    protected abstract loadJS(name: string, mod: M, newName?: string): Promise<void>;
 
     @bind private async remove(
         message: Zigbee2MQTTAPI['bridge/request/converter/remove'] | Zigbee2MQTTAPI['bridge/request/extension/remove'],
@@ -112,18 +131,21 @@ export default abstract class ExternalJSExtension<M> extends Extension {
         }
 
         const {name} = message;
+        const srcToBeRemoved = this.getFilePath(name, false, true);
         const toBeRemoved = this.getFilePath(name);
 
-        if (fs.existsSync(toBeRemoved)) {
-            await this.removeJS(name, this.loadModuleFromText(this.getFileCode(name), name));
+        if (fs.existsSync(srcToBeRemoved)) {
+            const mod = await import(this.getImportPath(srcToBeRemoved));
 
+            await this.removeJS(name, mod.default);
+            fs.rmSync(srcToBeRemoved, {force: true});
             fs.rmSync(toBeRemoved, {force: true});
             logger.info(`${name} (${toBeRemoved}) removed.`);
             await this.publishExternalJS();
 
             return utils.getResponse(message, {});
         } else {
-            return utils.getResponse(message, {}, `${name} (${toBeRemoved}) doesn't exists`);
+            return utils.getResponse(message, {}, `${name} (${srcToBeRemoved}) doesn't exists`);
         }
     }
 
@@ -135,32 +157,76 @@ export default abstract class ExternalJSExtension<M> extends Extension {
         }
 
         const {name, code} = message;
+        const srcFilePath = this.getFilePath(name, true, true);
+        let newName = name;
+
+        if (fs.existsSync(srcFilePath)) {
+            // if file already exist, version it to bypass node module caching
+            const versionMatch = name.match(/\.(\d+)\.(c|m)?js$/);
+
+            if (versionMatch) {
+                const version = parseInt(versionMatch[1], 10);
+                newName = name.replace(`.${version}.`, `.${version + 1}.`);
+            } else {
+                const ext = path.extname(name);
+                newName = name.replace(ext, `.1${ext}`);
+            }
+
+            // remove previous version
+            fs.rmSync(srcFilePath, {force: true});
+            fs.rmSync(this.getFilePath(name, true, false), {force: true});
+        }
+
+        const newSrcFilePath = this.getFilePath(newName, false /* already created above if needed */, true);
 
         try {
-            await this.loadJS(name, this.loadModuleFromText(code, name));
+            fs.writeFileSync(newSrcFilePath, code, 'utf8');
 
-            const filePath = this.getFilePath(name, true);
+            const mod = await import(this.getImportPath(newSrcFilePath));
 
-            fs.writeFileSync(filePath, code, 'utf8');
-            logger.info(`${name} loaded. Contents written to '${filePath}'.`);
+            await this.loadJS(name, mod.default, newName);
+            logger.info(`${newName} loaded. Contents written to '${newSrcFilePath}'.`);
+            // keep original in data folder synced
+            fs.writeFileSync(this.getFilePath(newName, true, false), code, 'utf8');
             await this.publishExternalJS();
 
             return utils.getResponse(message, {});
         } catch (error) {
-            return utils.getResponse(message, {}, `${name} contains invalid code: ${(error as Error).message}`);
+            fs.rmSync(newSrcFilePath, {force: true});
+            // NOTE: original in data folder doesn't get written if invalid
+
+            return utils.getResponse(message, {}, `${newName} contains invalid code: ${(error as Error).message}`);
         }
     }
 
     private async loadFiles(): Promise<void> {
         for (const extension of this.getFiles()) {
-            await this.loadJS(extension.name, this.loadModuleFromText(extension.code, extension.name));
+            const srcFilePath = this.getFilePath(extension.name, true, true);
+            const filePath = this.getFilePath(extension.name);
+
+            try {
+                fs.copyFileSync(filePath, srcFilePath);
+
+                const mod = await import(this.getImportPath(srcFilePath));
+
+                await this.loadJS(extension.name, mod.default);
+            } catch (error) {
+                // change ext so Z2M doesn't try to load it again and again
+                fs.renameSync(filePath, `${filePath}.invalid`);
+                fs.rmSync(srcFilePath, {force: true});
+
+                logger.error(
+                    `Invalid external ${this.mqttTopic} '${extension.name}' was ignored and renamed to prevent interference with Zigbee2MQTT.`,
+                );
+                logger.debug((error as Error).stack!);
+            }
         }
     }
 
     private async publishExternalJS(): Promise<void> {
         await this.mqtt.publish(
             `bridge/${this.mqttTopic}s`,
-            stringify(Array.from(this.getFiles())),
+            stringify(Array.from(this.getFiles(true))),
             {
                 retain: true,
                 qos: 0,
@@ -170,22 +236,8 @@ export default abstract class ExternalJSExtension<M> extends Extension {
         );
     }
 
-    private loadModuleFromText(moduleCode: string, name: string): M {
-        const moduleFakePath = path.join(__dirname, '..', '..', 'data', 'extension', name);
-        const sandbox: Context = {
-            require: require,
-            module: {},
-            console,
-            setTimeout,
-            clearTimeout,
-            setInterval,
-            clearInterval,
-            setImmediate,
-            clearImmediate,
-        };
-
-        runInNewContext(moduleCode, sandbox, moduleFakePath);
-
-        return sandbox.module.exports;
+    private getImportPath(filePath: string): string {
+        // prevent issues on Windows
+        return path.relative(__dirname, filePath).replaceAll('\\', '/');
     }
 }

test/assets/external_converters/mock-external-converter-multiple.js test/assets/external_converters/cjs/mock-external-converter-multiple.js

@@ -1,9 +1,11 @@
+const {posix} = require('node:path');
+
 const mockDevice = {
     mock: true,
     zigbeeModel: ['external_converter_device'],
     vendor: 'external',
     model: 'external_converter_device',
-    description: 'external',
+    description: posix.join('external', 'converter'),
     fromZigbee: [],
     toZigbee: [],
     exposes: [],

test/assets/external_converters/mock-external-converter.js test/assets/external_converters/cjs/mock-external-converter.js

@@ -0,0 +1,22 @@
+export default [
+    {
+        mock: 1,
+        model: 'external_converters_device_1',
+        zigbeeModel: ['external_converter_device_1'],
+        vendor: 'external_1',
+        description: 'external_1',
+        fromZigbee: [],
+        toZigbee: [],
+        exposes: [],
+    },
+    {
+        mock: 2,
+        model: 'external_converters_device_2',
+        zigbeeModel: ['external_converter_device_2'],
+        vendor: 'external_2',
+        description: 'external_2',
+        fromZigbee: [],
+        toZigbee: [],
+        exposes: [],
+    },
+];