Skip to content
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

fix: corrects resolve for win32 paths #436

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: corrects resolve for win32 paths #436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion .cspell.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@
"zipp",
"zippi",
"zizizi",
"codecov"
"codecov",
"obps",
"posix"
],
"ignorePaths": ["package.json", "yarn.lock", "coverage", "*.log"]
}
2 changes: 2 additions & 0 deletions lib/DescriptionFileUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,8 @@ function getField(content, field) {
*/
function cdUp(directory) {
if (directory === "/") return null;
if (directory.match(/^[a-zA-Z]:$/)) return null;
Copy link
Member

Choose a reason for hiding this comment

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

Why we need it? We have unix path internally...

Copy link
Author

Choose a reason for hiding this comment

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

Because it can be win32 path still. Of course, it has posix char separator, but absolute path can be resolved as X:/something/ for example and for this case - to prevent this function to go up


const i = directory.lastIndexOf("/"),
j = directory.lastIndexOf("\\");
const p = i < 0 ? j : j < 0 ? i : i < j ? j : i;
Expand Down
10 changes: 10 additions & 0 deletions lib/PnpPlugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

"use strict";

const { transformPathToPosix } = require("./util/path");

/** @typedef {import("./Resolver")} Resolver */
/** @typedef {import("./Resolver").ResolveStepHook} ResolveStepHook */
/** @typedef {import("./Resolver").ResolveRequest} ResolveRequest */
Expand Down Expand Up @@ -77,10 +79,18 @@ module.exports = class PnpPlugin {
return;
}

if (typeof resolution === "string") {
resolution = transformPathToPosix(resolution);
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Author

@davakh davakh Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

external path that can be received from pnpapi -> we transform it to path with posix separators in case we get path with win32 separator from pnpapi

}

if (resolveContext.fileDependencies) {
apiResolution = this.pnpApi.resolveToUnqualified("pnpapi", issuer, {
considerBuiltins: false
});

if (typeof apiResolution === "string") {
apiResolution = transformPathToPosix(apiResolution);
}
}
} catch (/** @type {unknown} */ error) {
if (
Expand Down
9 changes: 5 additions & 4 deletions lib/Resolver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ const {
normalize,
cachedJoin: join,
getType,
PathType
PathType,
transformPathToPosix
} = require("./util/path");

/** @typedef {import("./ResolverFactory").ResolveOptions} ResolveOptions */
Expand Down Expand Up @@ -505,8 +506,8 @@ class Resolver {
/** @type {ResolveRequest} */
const obj = {
context: context,
path: path,
request: request
path: transformPathToPosix(path),
request: transformPathToPosix(request)
};

/** @type {ResolveContextYield | undefined} */
Expand Down Expand Up @@ -535,7 +536,7 @@ class Resolver {
};
}

const message = `resolve '${request}' in '${path}'`;
const message = `resolve '${obj.request}' in '${obj.path}'`;

/**
* @param {ResolveRequest} result result
Expand Down
7 changes: 5 additions & 2 deletions lib/ResolverFactory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
const versions = require("process").versions;
const Resolver = require("./Resolver");
const { getType, PathType } = require("./util/path");
const {
normalizeOptionsForWindows
} = require("./util/win32-normalize-options");

const SyncAsyncFileSystemDecorator = require("./SyncAsyncFileSystemDecorator");

Expand Down Expand Up @@ -199,7 +202,7 @@ function createOptions(options) {
}
}

return {
return normalizeOptionsForWindows({
Copy link
Member

Choose a reason for hiding this comment

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

I am afraid it will be slow on big builds...

Copy link
Author

@davakh davakh Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, I'll take a look how it integrates with other libraries, maybe I'll find better place to normalize it. Don't really sure how often resolver factory can be called.

alias: normalizeAlias(options.alias),
fallback: normalizeAlias(options.fallback),
aliasFields: new Set(options.aliasFields),
Expand Down Expand Up @@ -267,7 +270,7 @@ function createOptions(options) {
preferRelative: options.preferRelative || false,
preferAbsolute: options.preferAbsolute || false,
restrictions: new Set(options.restrictions)
};
});
}

/**
Expand Down
3 changes: 1 addition & 2 deletions lib/RestrictionsPlugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
/** @typedef {import("./Resolver").ResolveStepHook} ResolveStepHook */

const slashCode = "/".charCodeAt(0);
const backslashCode = "\\".charCodeAt(0);
Copy link
Member

Choose a reason for hiding this comment

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

Can you describe?

Copy link
Author

Choose a reason for hiding this comment

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

We can keep it like this, but I thought about removing it because internally we should handle only posix separator char


/**
* @param {string} path path
Expand All @@ -20,7 +19,7 @@ const isInside = (path, parent) => {
if (!path.startsWith(parent)) return false;
if (path.length === parent.length) return true;
const charCode = path.charCodeAt(parent.length);
return charCode === slashCode || charCode === backslashCode;
return charCode === slashCode;
};

module.exports = class RestrictionsPlugin {
Expand Down
2 changes: 1 addition & 1 deletion lib/getPaths.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
*/
module.exports = function getPaths(path) {
if (path === "/") return { paths: ["/"], segments: [""] };
const parts = path.split(/(.*?[\\/]+)/);
const parts = path.split(/(.*?[/]+)/);
Copy link
Member

Choose a reason for hiding this comment

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

And this

Copy link
Author

Choose a reason for hiding this comment

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

Same idea as this: #436 (comment)

const paths = [path];
const segments = [parts[parts.length - 1]];
let part = parts[parts.length - 1];
Expand Down
20 changes: 16 additions & 4 deletions lib/util/path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ const CHAR_DOT = ".".charCodeAt(0);
const CHAR_COLON = ":".charCodeAt(0);

const posixNormalize = path.posix.normalize;
const winNormalize = path.win32.normalize;

/**
* @enum {number}
Expand Down Expand Up @@ -134,7 +133,7 @@ const normalize = p => {
case PathType.Empty:
return p;
case PathType.AbsoluteWin:
return winNormalize(p);
return posixNormalize(p);
case PathType.Relative: {
const r = posixNormalize(p);
return getType(r) === PathType.Relative ? r : `./${r}`;
Expand All @@ -156,15 +155,15 @@ const join = (rootPath, request) => {
case PathType.AbsolutePosix:
return posixNormalize(request);
case PathType.AbsoluteWin:
return winNormalize(request);
return posixNormalize(request);
}
switch (getType(rootPath)) {
case PathType.Normal:
case PathType.Relative:
case PathType.AbsolutePosix:
return posixNormalize(`${rootPath}/${request}`);
case PathType.AbsoluteWin:
return winNormalize(`${rootPath}\\${request}`);
return posixNormalize(`${rootPath}/${request}`);
}
switch (requestType) {
case PathType.Empty:
Expand Down Expand Up @@ -201,3 +200,16 @@ const cachedJoin = (rootPath, request) => {
return cacheEntry;
};
exports.cachedJoin = cachedJoin;

/**
* @param {string} rawPath node.js path
* @returns {string} normalized for windows, same for posix paths
*/
const transformPathToPosix = rawPath => {
if (!rawPath) return rawPath;

if (path.sep === "/")
return rawPath; // Don't even transform if path.sep is posix
else return rawPath.split("\\").join("/");
};
Copy link
Member

Choose a reason for hiding this comment

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

I am very worried about the performance here and that we will have different behavior on windws and nix systems

Copy link
Author

@davakh davakh Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree about the performance part. About the behavior: it's already different and based on os.

It's possible with this PR to test how different behavior is between Unix and Windows with the following instruction:

  • revert changes from library itself, keep the changes for tests
  • mock transferPathToPosix function in test/util/path-separator.js because it exists in tests only for assertion and handles assertion for changes exclusively in current pull request (it removes the conversion from \\ to / provided by changes in library which we reverted)
  • run the tests on Windows (Unix should run successfully)

exports.transformPathToPosix = transformPathToPosix;
108 changes: 108 additions & 0 deletions lib/util/win32-normalize-options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
/*
MIT License http://www.opensource.org/licenses/mit-license.php
Copy link
Author

@davakh davakh Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

question: not sure, what I should write here? should I add same author (Author Tobias Koppers @sokra) as other files? sorry, not so much contributing... 🙃

Copy link
Member

Choose a reason for hiding this comment

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

@davakh You can write own name here

*/

"use strict";

const { transformPathToPosix } = require("./path");

/** @typedef {import("../AliasPlugin").AliasOption} AliasOptionEntry */
/** @typedef {import("../Resolver").ResolveOptions} ResolveOptions */

/**
* @param {AliasOptionEntry[]} aliasOptions alias options
* @returns {AliasOptionEntry[]} normalized for win32 aliases
*/
function normalizeWindowsAliasOption(aliasOptions) {
return aliasOptions.map(aliasOption => {
let newAliasOption = aliasOption.alias;

if (typeof newAliasOption !== "boolean") {
newAliasOption = normalizeStringOrArrayOfStrings(newAliasOption);
}

return {
...aliasOption,
name: normalizeStringOrArrayOfStrings(aliasOption.name),
alias: newAliasOption
};
});
}

/**
* @param {Set<string | string[]> | Set<string> | Set<string[]>} rawSet alias
* @returns {*} normalized fon win32 sets of string or string[]
*/
function normalizeStringifiedSets(rawSet) {
const normalizedSet = new Set();
rawSet.forEach(item => {
normalizedSet.add(normalizeStringOrArrayOfStrings(item));
});
return normalizedSet;
}

/**
* @param {string | string[]} str str
* @returns {*} normalized str
*/
function normalizeStringOrArrayOfStrings(str) {
return Array.isArray(str)
? str.map(transformPathToPosix)
: transformPathToPosix(str);
}

/**
* @param {ResolveOptions} resolveOptions input options
* @returns {ResolveOptions} output options
*/
function normalizeOptionsForWindows(resolveOptions) {
// List of all options that can be passed with win32 path separator
resolveOptions.alias = normalizeWindowsAliasOption(resolveOptions.alias);
resolveOptions.fallback = normalizeWindowsAliasOption(
resolveOptions.fallback
);
resolveOptions.aliasFields = normalizeStringifiedSets(
resolveOptions.aliasFields
);
resolveOptions.extensionAlias = resolveOptions.extensionAlias.map(
aliasOption => ({
...aliasOption,
alias: normalizeStringOrArrayOfStrings(aliasOption.alias)
})
);
resolveOptions.conditionNames = normalizeStringifiedSets(
resolveOptions.conditionNames
);
resolveOptions.descriptionFiles = normalizeStringOrArrayOfStrings(
resolveOptions.descriptionFiles
);
resolveOptions.exportsFields = normalizeStringifiedSets(
resolveOptions.exportsFields
);
resolveOptions.importsFields = normalizeStringifiedSets(
resolveOptions.importsFields
);
resolveOptions.extensions = normalizeStringifiedSets(
resolveOptions.extensions
);
resolveOptions.modules = Array.isArray(resolveOptions.modules)
? resolveOptions.modules.map(item => normalizeStringOrArrayOfStrings(item))
: resolveOptions.modules;
resolveOptions.mainFiles = normalizeStringifiedSets(resolveOptions.mainFiles);
resolveOptions.roots = normalizeStringifiedSets(resolveOptions.roots);

const newRestrictions = new Set();
resolveOptions.restrictions.forEach(restrict => {
if (typeof restrict === "string") {
newRestrictions.add(normalizeStringOrArrayOfStrings(restrict));
} else {
// regexp
newRestrictions.add(restrict);
}
});
resolveOptions.restrictions = newRestrictions;

return resolveOptions;
}

exports.normalizeOptionsForWindows = normalizeOptionsForWindows;
44 changes: 26 additions & 18 deletions test/CachedInputFileSystem.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const { CachedInputFileSystem } = require("../");
const path = require("path");
const url = require("url");
const { obps, absoluteOsBasedPath } = require("./util/path-separator");

describe("CachedInputFileSystem OperationMergerBackend ('stat' and 'statSync')", () => {
let fs;
Expand Down Expand Up @@ -423,28 +424,35 @@ describe("CachedInputFileSystem CacheBackend", () => {
});

it("should purge readdir correctly", function (done) {
fs.readdir("/test/path", (err, r) => {
fs.readdir(`${absoluteOsBasedPath}test${obps}path`, (err, r) => {
expect(r[0]).toEqual("0");
fs.purge(["/test/path/sub/path"]);
fs.readdir("/test/path", (err, r) => {
fs.purge([`${absoluteOsBasedPath}test${obps}path${obps}sub${obps}path`]);
fs.readdir(`${absoluteOsBasedPath}test${obps}path`, (err, r) => {
expect(r[0]).toEqual("0");
fs.purge(["/test/path/sub"]);
fs.readdir("/test/path", (err, r) => {
fs.purge([`${absoluteOsBasedPath}test${obps}path${obps}sub`]);
fs.readdir(`${absoluteOsBasedPath}test${obps}path`, (err, r) => {
expect(r[0]).toEqual("1");
fs.purge(["/test/path"]);
fs.readdir("/test/path", (err, r) => {
fs.purge([`${absoluteOsBasedPath}test${obps}path`]);
fs.readdir(`${absoluteOsBasedPath}test${obps}path`, (err, r) => {
expect(r[0]).toEqual("2");
fs.purge([url.pathToFileURL("/test/path")]);
fs.readdir("/test/path", (err, r) => {
fs.purge([
url.pathToFileURL(`${absoluteOsBasedPath}test${obps}path`)
]);
fs.readdir(`${absoluteOsBasedPath}test${obps}path`, (err, r) => {
expect(r[0]).toEqual("2");
fs.purge(Buffer.from("/test/path"));
fs.readdir("/test/path", (err, r) => {
fs.purge(Buffer.from(`${absoluteOsBasedPath}test${obps}path`));
fs.readdir(`${absoluteOsBasedPath}test${obps}path`, (err, r) => {
expect(r[0]).toEqual("3");
fs.purge([Buffer.from("/test/path")]);
fs.readdir("/test/path", (err, r) => {
expect(r[0]).toEqual("4");
done();
});
fs.purge([
Buffer.from(`${absoluteOsBasedPath}test${obps}path`)
]);
fs.readdir(
`${absoluteOsBasedPath}test${obps}path`,
(err, r) => {
expect(r[0]).toEqual("4");
done();
}
);
});
});
});
Expand Down Expand Up @@ -475,7 +483,7 @@ describe("CachedInputFileSystem CacheBackend and Node.JS filesystem", () => {
fs = new CachedInputFileSystem(require("fs"), 1);
});

const file = path.resolve(__dirname, "./fixtures/abc.txt");
const file = path.resolve(__dirname, `.${obps}fixtures${obps}abc.txt`);

it("should work with string async", function (done) {
fs.readFile(file, (err, r) => {
Expand Down Expand Up @@ -533,7 +541,7 @@ describe("CachedInputFileSystem OperationMergerBackend and Node.JS filesystem",
fs = new CachedInputFileSystem(require("fs"), 0);
});

const file = path.resolve(__dirname, "./fixtures/abc.txt");
const file = path.resolve(__dirname, `.${obps}fixtures${obps}abc.txt`);

it("should work with string async", function (done) {
fs.readFile(file, (err, r) => {
Expand Down
Loading