Skip to content

feat(apps): export ENGINE_VERSION from definition layer #40183

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

Closed
d-gubert wants to merge 3 commits into from
Closed

feat(apps): export ENGINE_VERSION from definition layer #40183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
721cd6c
feat(apps-engine): export ENGINE_VERSION from definition layer
d-gubert Apr 16, 2026
6f5aed5
fix: path detection for version.ts
d-gubert Apr 22, 2026
048d05b
docs: apps-engine-migration.md
d-gubert May 4, 2026
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
1 change: 1 addition & 0 deletions docs/apps-engine-migration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,4 @@ Besides that, migrating app management code away from a public package enables u
To make this migration easier to understand and review, we're using a stacked PR approach on Github - similar to a feature branch but disallowing sibling PRs. They are:

- [40395](https://github.com/RocketChat/Rocket.Chat/pull/40395) The feature branch itself. It will accumulate the changes of the whole stack.
- [40183](https://github.com/RocketChat/Rocket.Chat/pull/40183) Replaces `AppPackageParser.getEngineVersion()` - which resolved the version by traversing the filesystem relative to `__dirname` - with a direct import of `ENGINE_VERSION`. This will support the migration of the `AppPackageParser` class itself.
16 changes: 16 additions & 0 deletions packages/apps-engine/src/definition/version.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* The version of the Apps-Engine package.
* Consumed by host-side code (e.g. AppPackageParser) to validate app compatibility
* without relying on filesystem path traversal.
*
* Uses require() instead of a static import so TypeScript does not resolve the path
* at compile time.
*
* When running for tests, using ts-node, package.json is located two levels above the current file.
* When running in production, package.json is located one level above the compiled version of this file.
*/
const runningFromSource = __dirname.endsWith('src/definition');
const requirePath = runningFromSource ? '../../package.json' : '../package.json';
Comment on lines +12 to +13

@coderabbitai coderabbitai Bot Apr 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Path heuristic is OS-dependent — breaks on Windows.

__dirname on Windows uses backslashes (e.g. C:\…\src\definition), so endsWith('src/definition') will always be false there and the wrong relative path will be selected when running tests via ts-node on Windows. Even if Windows is not a primary target, it's a one-line fix:

🔧 Suggested fix 
-const runningFromSource = __dirname.endsWith('src/definition');
-const requirePath = runningFromSource ? '../../package.json' : '../package.json';
+const runningFromSource = __dirname.split(/[\\/]/).slice(-2).join('/') === 'src/definition';
+const requirePath = runningFromSource ? '../../package.json' : '../package.json';

Or use path.basename(path.dirname(__dirname)) === 'src' && path.basename(__dirname) === 'definition'.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const runningFromSource = __dirname.endsWith('src/definition');
const requirePath = runningFromSource ? '../../package.json' : '../package.json';
const runningFromSource = __dirname.split(/[\\/]/).slice(-2).join('/') === 'src/definition';
const requirePath = runningFromSource ? '../../package.json' : '../package.json';
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/apps-engine/src/definition/version.ts` around lines 12 - 13, The
OS-dependent dirname check using __dirname.endsWith('src/definition') can fail
on Windows; update the runningFromSource logic to use path utilities instead
(import path) — e.g., determine runningFromSource by checking
path.basename(path.dirname(__dirname)) === 'src' && path.basename(__dirname) ===
'definition' or by normalizing separators with path.join/normalize, then set
requirePath accordingly; modify the runningFromSource and requirePath
calculation (symbols: runningFromSource, __dirname, requirePath) to use path
methods so the correct package.json path is chosen cross-platform.


// eslint-disable-next-line import/no-dynamic-require, @typescript-eslint/no-require-imports -- Paths are bounded, we just need to decide which package.json file to target
export const ENGINE_VERSION: string = require(requirePath).version;

@coderabbitai coderabbitai Bot May 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

ENGINE_VERSION can silently be undefined despite the string annotation.

require(requirePath) returns any, so if the loaded package.json is missing a version field, ENGINE_VERSION is exported as undefined. Downstream semver comparisons (e.g. in AppPackageParser) would then fail cryptically rather than at the point of bad data.

Add a runtime guard at module load time:

🛡️ Proposed fix 
-// eslint-disable-next-line import/no-dynamic-require, `@typescript-eslint/no-require-imports` -- Paths are bounded, we just need to decide which package.json file to target
-export const ENGINE_VERSION: string = require(requirePath).version;
+// eslint-disable-next-line import/no-dynamic-require, `@typescript-eslint/no-require-imports` -- Paths are bounded, we just need to decide which package.json file to target
+const { version }: { version?: string } = require(requirePath);
+if (!version) {
+    throw new Error(`[apps-engine] Could not read version from ${requirePath}`);
+}
+export const ENGINE_VERSION: string = version;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const ENGINE_VERSION: string = require(requirePath).version;
// eslint-disable-next-line import/no-dynamic-require, `@typescript-eslint/no-require-imports` -- Paths are bounded, we just need to decide which package.json file to target
const { version }: { version?: string } = require(requirePath);
if (!version) {
throw new Error(`[apps-engine] Could not read version from ${requirePath}`);
}
export const ENGINE_VERSION: string = version;
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/apps-engine/src/definition/version.ts` at line 16, The export
ENGINE_VERSION currently reads require(requirePath).version which can be
undefined at runtime; update module load to read the package JSON via
require(requirePath), validate that the returned object has a non-empty string
"version" property, and if not throw a clear Error (or assert) so consumers fail
fast; reference the ENGINE_VERSION constant and requirePath variable when
implementing this guard and assign ENGINE_VERSION only after the validation
passes.

27 changes: 3 additions & 24 deletions packages/apps-engine/src/server/compiler/AppPackageParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import * as fs from 'fs';
import * as path from 'path';

import * as AdmZip from 'adm-zip';
Expand All @@ -8,18 +7,15 @@ import { v4 as uuidv4 } from 'uuid';
import { AppImplements } from '.';
import type { IParseAppPackageResult } from './IParseAppPackageResult';
import type { IAppInfo } from '../../definition/metadata/IAppInfo';
import { ENGINE_VERSION } from '../../definition/version';
import { RequiredApiVersionError } from '../errors';

export class AppPackageParser {
public static uuid4Regex = /^[0-9a-fA-f]{8}-[0-9a-fA-F]{4}-4[0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}$/;
public static uuid4Regex = /^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-4[0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}$/;

private allowedIconExts: Array<string> = ['.png', '.jpg', '.jpeg', '.gif'];

private appsEngineVersion: string;

constructor() {
this.appsEngineVersion = this.getEngineVersion();
}
private appsEngineVersion: string = ENGINE_VERSION;

public async unpackageApp(appPackage: Buffer): Promise<IParseAppPackageResult> {
const zip = new AdmZip(appPackage);
Expand Down Expand Up @@ -143,21 +139,4 @@ export class AppPackageParser {

return `data:image/${ext.replace('.', '')};base64,${base64}`;
}

private getEngineVersion(): string {
const devLocation = path.join(__dirname, '../../../package.json');
const prodLocation = path.join(__dirname, '../../package.json');

let info: { version: string };

if (fs.existsSync(devLocation)) {
info = JSON.parse(fs.readFileSync(devLocation, 'utf8'));
} else if (fs.existsSync(prodLocation)) {
info = JSON.parse(fs.readFileSync(prodLocation, 'utf8'));
} else {
throw new Error('Could not find the Apps TypeScript Definition Package Version!');
}

return info.version.replace(/^[^0-9]/, '').split('-')[0];
}
}
1 change: 1 addition & 0 deletions packages/apps-engine/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"emitDecoratorMetadata": true,
"experimentalDecorators": true,
"moduleResolution": "node",
"resolveJsonModule": true,
"types": ["node"],
"lib": ["es2017", "dom"],
"rootDir": "./src",
Expand Down
Loading