fix: corrects resolve for win32 paths by davakh · Pull Request #436 · webpack/enhanced-resolve

davakh · 2024-10-30T20:31:38Z

fixes #401

accidentaly closed previous PR: #402

davakh · 2024-10-30T20:39:41Z

test/pnp.test.js

 	it("should resolve namespaced module names", done => {
-		pnpApi.mocks.set("@user/pkg", path.resolve(fixture, "pkg"));
-		resolver.resolve({}, __dirname, "@user/pkg", {}, (err, result) => {
+		pnpApi.mocks.set(`@user${posixSep}pkg`, path.resolve(fixture, "pkg"));


thoughts: i'm not sure about using posixSep in this place, because it should use osBasedPathSeparator (obps) based on reasoning from /test/util/path-separator description of this variables, but this test is mocking pnpapi and resolving it also in this file, so that's the place where I want to think about one more time. Probably, will try to take a look on pnpapi

davakh · 2024-10-30T20:42:49Z

test/util/path-separator.js

+    absoluteOsBasedPath,
+    absoluteOsBasedResolvedPath,
+		posixSep: posixPathSeparator,
+		obps: osBasedPathSeparator,


thoughts: I wanted to use fullname, but this entity being used too often in tests, so I decided to make name shorter

davakh · 2024-10-30T20:47:48Z

lib/util/win32-normalize-options.js

@@ -0,0 +1,108 @@
+/*
+	MIT License http://www.opensource.org/licenses/mit-license.php


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... 🙃

@davakh You can write own name here

davakh · 2024-10-30T21:30:07Z

test/yield.test.js

+// Functions below sometime handle transferPathToPosix and based on reading of test file you can see, that
+// sometimes absolute paths started with obps (osBasedPathSeparator) and not absoluteOsBasedPath. That's because we use path.join too much in this test,
+// and it works tricky on platforms:
+// > path.posix.join('/abc/ab', '/abc/de') -> '/abc/ab/abc/de' (correct path after all, )


nitpick: I'll fix it with following changes after review

alexander-akait · 2024-11-01T10:52:36Z

lib/ResolverFactory.js

 	}

-	return {
+	return normalizeOptionsForWindows({


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

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.

alexander-akait

Why just don't coonvert it to unix path and work internaly only with /? I just don't understand some of the changes.

alexander-akait · 2024-11-01T10:55:50Z

lib/DescriptionFileUtils.js

 */
 function cdUp(directory) {
 	if (directory === "/") return null;
+	if (directory.match(/^[a-zA-Z]:$/)) return null;


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

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

alexander-akait · 2024-11-01T10:56:00Z

lib/PnpPlugin.js

 					}

+					if (typeof resolution === "string") {
+						resolution = transformPathToPosix(resolution);


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

alexander-akait · 2024-11-01T10:56:20Z

lib/RestrictionsPlugin.js

 /** @typedef {import("./Resolver").ResolveStepHook} ResolveStepHook */

 const slashCode = "/".charCodeAt(0);
-const backslashCode = "\\".charCodeAt(0);


Can you describe?

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

alexander-akait · 2024-11-01T10:56:28Z

lib/getPaths.js

 module.exports = function getPaths(path) {
 	if (path === "/") return { paths: ["/"], segments: [""] };
-	const parts = path.split(/(.*?[\\/]+)/);
+	const parts = path.split(/(.*?[/]+)/);


Same idea as this: #436 (comment)

davakh · 2024-11-01T21:47:26Z

Why just don't coonvert it to unix path and work internaly only with /? I just don't understand some of the changes.

That's what I did, that's how it works right now in this PR: we convert the external paths, we can get it in resolver or with ResolverFactory options and work internally only with posix / path separator.

So, actually, we convert win32 path to posix path only in case of ResolverFactory options or resolver usage. Yes, there can be some exceptions: f.e. PnpPlugin - it gets new paths from the pnpapi so it requires to handle this case separately.

UPDATE: Okay, I think that I got where the misunderstanding comes from: I'm thinking more about using win32 paths still but with posix path separator - because node.js supports this. So for me, it means we work with relative type of path (win32/posix) based on OS, but we have posix separator in win32 paths instead of win32 separator and it's still win32 type path.

alexander-akait · 2024-12-04T14:16:51Z

lib/util/path.js

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


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

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)

fix: corrects resolve for win32 paths

2a7b9b9

webpack-bot added the PR: unreviewed label Oct 30, 2024

davakh mentioned this pull request Oct 30, 2024

feat: add support for win32 paths #402

Closed

davakh commented Oct 30, 2024

View reviewed changes

alexander-akait reviewed Nov 1, 2024

View reviewed changes

alexander-akait reviewed Dec 4, 2024

View reviewed changes

		@@ -0,0 +1,108 @@
		/*
		MIT License http://www.opensource.org/licenses/mit-license.php

Uh oh!

Conversation

davakh commented Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davakh Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davakh Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davakh Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davakh Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davakh Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davakh Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davakh commented Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davakh Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davakh commented Oct 30, 2024 •

edited

Loading

davakh Oct 30, 2024 •

edited

Loading

davakh Oct 30, 2024 •

edited

Loading

davakh Oct 30, 2024 •

edited

Loading

davakh Oct 30, 2024 •

edited

Loading

davakh Nov 1, 2024 •

edited

Loading

davakh Nov 1, 2024 •

edited

Loading

davakh commented Nov 1, 2024 •

edited

Loading

davakh Dec 4, 2024 •

edited

Loading