Skip to content

refactor(index): use path.dirname over regex to resolve bin path #548

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

Open
Fdawgs wants to merge 2 commits into
base: main
Choose a base branch
from
+12 −13
Open

refactor(index): use path.dirname over regex to resolve bin path #548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5246d9e
refactor(index): use `path.dirname` over regex to resolve bin path
Fdawgs Feb 24, 2026
5b727e2
test(index): use dirname in tests
Fdawgs Feb 24, 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
15 changes: 7 additions & 8 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

const { spawn, spawnSync } = require("node:child_process");
const { open } = require("node:fs/promises");
const { normalize, resolve: pathResolve } = require("node:path");
const { dirname, normalize, resolve: pathResolve } = require("node:path");
const { platform } = require("node:process");
const freeze = require("ice-barrage");
const { gt, lt } = require("semver");
Expand All @@ -19,7 +19,6 @@ const RTF_MAGIC_BUFFER = Buffer.from(RTF_MAGIC_NUMBER);
const RTF_MAGIC_NUMBER_LENGTH = RTF_MAGIC_NUMBER.length;

// Cache immutable regex as they are expensive to create and garbage collect
const UNRTF_PATH_REG = /(.+)unrtf/u;
// UnRTF version output is inconsistent between versions but always starts with the semantic version number
const UNRTF_VERSION_REG = /^(\d{1,2}\.\d{1,2}\.\d{1,2})/u;

Expand Down Expand Up @@ -218,15 +217,15 @@ class UnRTF {
/* istanbul ignore next: requires specific OS */
const which = spawnSync(platform === "win32" ? "where" : "which", [
"unrtf",
]).stdout.toString();
const unrtfPath = UNRTF_PATH_REG.exec(which)?.[1];

if (unrtfPath) {
this.#unrtfPath = unrtfPath;
])
.stdout.toString()
.trim();
if (which) {
this.#unrtfPath = dirname(which);
}
Comment on lines +220 to 225
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

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

if (which) { this.#unrtfPath = dirname(which); } will also treat any non-empty diagnostic message on stdout as a path (e.g., some where/which failure outputs), resulting in dirname(...) === "." and a misleading later "Unable to determine UnRTF version" error. Consider gating this assignment on spawnSync(...).status === 0 and/or validating that the selected line is an absolute/expected executable path before setting #unrtfPath.

Copilot uses AI. Check for mistakes.
Comment on lines 218 to 225
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

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

where on Windows can return multiple matching paths (one per line). Calling dirname() on the raw multi-line string will produce an invalid path and can prevent the win32 fallback from running. Consider splitting stdout by newlines and selecting the first valid path (or the first line that ends with unrtf/unrtf.exe) before applying dirname().

Copilot uses AI. Check for mistakes.

/* istanbul ignore next: requires specific OS */
if (platform === "win32" && !unrtfPath) {
if (platform === "win32" && !this.#unrtfPath) {
Comment on lines 218 to +228
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

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

The constructor logic now depends on parsing where/which output with trimming + dirname(), but the existing tests only cover empty stdout and a single-path stdout. Add a unit test that mocks win32 where returning multiple lines and/or a non-empty "not found" message, to ensure #unrtfPath is set (or remains empty to trigger the win32 fallback) correctly.

Copilot uses AI. Check for mistakes.
try {
// @ts-ignore: Optional dependency
// eslint-disable-next-line n/global-require -- Conditional require
Expand Down
10 changes: 5 additions & 5 deletions test/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

const { execFile, spawnSync } = require("node:child_process");
const { unlink } = require("node:fs/promises");
const { join, normalize, sep } = require("node:path");
const { dirname, join, normalize, sep } = require("node:path");
const { platform } = require("node:process");
const { promisify } = require("node:util");
const {
Expand Down Expand Up @@ -42,10 +42,10 @@ const originalProcess = jest.requireActual("node:process");
* @throws {Error} If the OS is not supported or the binaries are not found.
*/
function getTestBinaryPath() {
const which = spawnSync(platform === "win32" ? "where" : "which", [
"unrtf",
]).stdout.toString();
let unrtfPath = /(.+)unrtf/u.exec(which)?.[1];
const which = spawnSync(platform === "win32" ? "where" : "which", ["unrtf"])
.stdout.toString()
.trim();
let unrtfPath = dirname(which);

if (platform === "win32" && !unrtfPath) {
// @ts-ignore: Optional dependency
Expand Down
Loading