-
Notifications
You must be signed in to change notification settings - Fork 50
fix(utils,core,action): normalize path handling and test guards #2809
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| --- | ||
| "@node-minify/core": patch | ||
| "@node-minify/utils": patch | ||
| --- | ||
|
|
||
| fix: normalize cross-platform path handling in core and utils | ||
|
|
||
| Improves Windows/POSIX path compatibility for output directory resolution, wildcard handling, and public folder/minified path generation. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| import { mkdir } from "node:fs/promises"; | ||
| import path from "node:path"; | ||
| import type { Compressor, Settings } from "@node-minify/types"; | ||
| import { afterEach, describe, expect, test, vi } from "vitest"; | ||
|
|
||
| vi.mock("node:fs/promises", () => ({ | ||
| mkdir: vi.fn().mockResolvedValue(undefined), | ||
| })); | ||
|
|
||
| vi.mock("@node-minify/utils", async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import("@node-minify/utils")>(); | ||
| return { | ||
| ...actual, | ||
| compressSingleFile: vi.fn().mockResolvedValue("ok"), | ||
| }; | ||
| }); | ||
|
|
||
| import { compressSingleFile } from "@node-minify/utils"; | ||
| import { compress } from "../src/compress.ts"; | ||
|
|
||
| describe("compress path handling", () => { | ||
| afterEach(() => { | ||
| vi.clearAllMocks(); | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| test("should create directory for output paths using backslashes", async () => { | ||
| const compressor: Compressor = async () => ({ code: "ok" }); | ||
| const settings: Settings = { | ||
| compressor, | ||
| input: "input.js", | ||
| output: "nested\\dir\\file.min.js", | ||
| }; | ||
|
|
||
| await compress(settings); | ||
|
|
||
| expect(vi.mocked(mkdir)).toHaveBeenCalledWith("nested\\dir", { | ||
| recursive: true, | ||
| }); | ||
| expect(vi.mocked(compressSingleFile)).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| test("should preserve dirname separators before mkdir", async () => { | ||
| vi.spyOn(path, "dirname").mockReturnValue("nested\\dir"); | ||
|
|
||
| const compressor: Compressor = async () => ({ code: "ok" }); | ||
| const settings: Settings = { | ||
| compressor, | ||
| input: "input.js", | ||
| output: "nested\\dir\\file.min.js", | ||
| }; | ||
|
|
||
| await compress(settings); | ||
|
|
||
| expect(vi.mocked(mkdir)).toHaveBeenCalledWith("nested\\dir", { | ||
| recursive: true, | ||
| }); | ||
| expect(vi.mocked(compressSingleFile)).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| test("should preserve mixed-separator directory paths", async () => { | ||
| const compressor: Compressor = async () => ({ code: "ok" }); | ||
| const settings: Settings = { | ||
| compressor, | ||
| input: "input.js", | ||
| output: "tmp\\sub/out.js", | ||
| }; | ||
|
|
||
| await compress(settings); | ||
|
|
||
| expect(vi.mocked(mkdir)).toHaveBeenCalledWith("tmp\\sub", { | ||
| recursive: true, | ||
| }); | ||
| expect(vi.mocked(compressSingleFile)).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| */ | ||
|
|
||
| import { mkdir } from "node:fs/promises"; | ||
| import path from "node:path"; | ||
| /** | ||
| * Module dependencies. | ||
| */ | ||
|
|
@@ -105,13 +106,13 @@ async function createDirectory(filePath: string | string[]) { | |
| const paths = Array.isArray(filePath) ? filePath : [filePath]; | ||
| const uniqueDirs = new Set<string>(); | ||
|
|
||
| for (const path of paths) { | ||
| if (typeof path !== "string") { | ||
| for (const outputPath of paths) { | ||
| if (typeof outputPath !== "string") { | ||
| continue; | ||
| } | ||
|
|
||
| // Extract directory path | ||
| const dirPath = path.substring(0, path.lastIndexOf("/")); | ||
| // Use platform dirname first, then fallback for Windows-style separators on POSIX. | ||
| const dirPath = getDirectoryPath(outputPath); | ||
|
|
||
| // Early return if no directory path | ||
| if (!dirPath) { | ||
|
|
@@ -126,3 +127,22 @@ async function createDirectory(filePath: string | string[]) { | |
| Array.from(uniqueDirs).map((dir) => mkdir(dir, { recursive: true })) | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Resolve the directory path from an output file path. | ||
| * @param outputPath Full path to the output file | ||
| * @returns A directory path when resolvable, or an empty string | ||
| */ | ||
| function getDirectoryPath(outputPath: string): string { | ||
| const dirPath = path.dirname(outputPath); | ||
| if (dirPath && dirPath !== ".") { | ||
| return dirPath; | ||
| } | ||
|
|
||
| const windowsDirPath = path.win32.dirname(outputPath); | ||
| if (windowsDirPath && windowsDirPath !== ".") { | ||
| return windowsDirPath; | ||
| } | ||
|
Comment on lines
+136
to
+145
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Windows absolute paths break
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/core/src/compress.ts
Line: 136:145
Comment:
**Windows absolute paths break**
`getDirectoryPath()` normalizes by doing `replaceAll("\\", "/")` before calling `mkdir()`. On Windows, this turns e.g. `C:\\out\\file.min.js` into `C:/out`, which is not a valid Windows path for `fs.mkdir` (it can be interpreted as a relative path and fail). Since this function is specifically meant to support Windows-style output paths, the normalization should preserve Windows absolute-path semantics (or avoid rewriting separators entirely and let Node handle native separators).
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| return ""; | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
Uh oh!
There was an error while loading. Please reload this page.