fix: github issue #4439: case insensitive compare of file paths.

[lovettchris]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:

resolves #4439

Description

On windows the string returned from await fs.realpath(fileName) might contain lower case drive letter while the string returned from path.resolve(__dirname, ...) can return a different case, which causes direct string comparisons using === to fail. This fix adds a win32 specific implementation of a helper function isSamePath that ensures the comparison is case insensitive on a win32 platform.

[lukastaegert]

Do you think we can add a test for this, after all, tests are also running on Windows. There is a special test file just for loadConfigFile, or maybe a test case in the tests/cli folder?

[codecov]

Codecov Report

Merging #4440 (97ffdc6) into master (35cbfae) will decrease coverage by 0.02%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master    #4440      +/-   ##
==========================================
- Coverage   98.77%   98.74%   -0.03%     
==========================================
  Files         205      205              
  Lines        7326     7329       +3     
  Branches     2079     2080       +1     
==========================================
+ Hits         7236     7237       +1     
- Misses         33       34       +1     
- Partials       57       58       +1     

Impacted Files	Coverage Δ
src/utils/path.ts	100.00% <ø> (ø)
cli/run/loadConfigFile.ts	92.72% <50.00%> (-3.43%)	⬇️
src/utils/sanitizeFileName.ts	80.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35cbfae...97ffdc6. Read the comment docs.

[lovettchris]

Do you think we can add a test for this, after all, tests are also running on Windows. There is a special test file just for loadConfigFile, or maybe a test case in the tests/cli folder?

yeah, I've been having a similar conversation over here: rollup/plugins#1134. I can certainly use that trick to make all the rollup tests fails in the same way. Just not sure how to do that in the CI VM...

[lovettchris]

Note there may be one more error lerking in the code because when I force the default drive letter to be wrong (using the tricks I mentioned in rollup/plugins#1134, then npm run build also fails as follows:

PS: and this is after I patched the older version 2.67.1 of node_modules\rollup. Interesting that rollup is used to build rollup :-) Must have been an interesting bootstrapping process...

[dnalborczyk]

On windows the string returned from await fs.realpath(fileName) might contain lower case drive letter while the string returned from path.resolve(__dirname, ...) can return a different case, which causes direct string comparisons using === to fail.

I wonder if this is a node.js bug?

[dnalborczyk]

btw @lukastaegert

this logic: https://github.com/rollup/rollup/pull/4440/files#diff-167a57161916452cc80dedbc71446dc3d1fdd43e8db6304525f7990b4759f57dR118

requiredFileName === resolvedFileName

is also responsible for this issue: #4465 (confirmed) as well as likely this one: #4446 (not confirmed)

but in this case both variables represent different paths, one is the 'real path', and the other the 'mounted path' (subst). this PR will not address this. I wonder if there's a better solution to fix both issues?

I also wonder now how this comparison logic works on mounted drives on macos/linux. 🤔

I also tried to figure out why this comparison is in place, but didn't get anywhere. the entire logic of reading just a config file seems to be overly complicated and convoluted on first sight. (not judging). I imagine it grew organically without much refactoring and one of the obstacles is also trying to read many formats .js (as cjs and esm), .cjs, .mjs (and .ts?) configs. but it still feels like the entire logic needs a bit of an overhaul.

it also appears that this is also a good candidate for a v3 refactoring (regarding proper cjs and esm parity with node.js, which it looks like rollup is doing it's own thing so to speak).

[pos777]

I got the same error and found that the loadConfigFromBundledFile function replaces the default loader for the resolvedFileName file (requiredFileName === resolvedFileName) while the fileName file is required (const config = getDefaultFromCjs(require(fileName))):

rollup/cli/run/loadConfigFile.ts

Lines 106 to 124 in 931a199

	async function loadConfigFromBundledFile(fileName: string, bundledCode: string): Promise<unknown> {
	const resolvedFileName = await fs.realpath(fileName);
	const extension = extname(resolvedFileName);
	const defaultLoader = require.extensions[extension];
	require.extensions[extension] = (module: NodeModule, requiredFileName: string) => {
	if (requiredFileName === resolvedFileName) {
	(module as NodeModuleWithCompile)._compile(bundledCode, requiredFileName);
	} else {
	if (defaultLoader) {
	defaultLoader(module, requiredFileName);
	}
	}
	};
	delete require.cache[resolvedFileName];
	try {
	const config = getDefaultFromCjs(require(fileName));
	require.extensions[extension] = defaultLoader;
	return config;
	} catch (err: any) {

In the problematic scenario the resolvedFileName and the fileName differ in case.

Everything works as expected if I change const config = getDefaultFromCjs(require(fileName)) to const config = getDefaultFromCjs(require(resolvedFileName)). The #4465 scenario works as well. But I don't know if it is correct in all cases.

UPD: I have prepared #4501 to demonstrate an alternative fix. @lovettchris, @dnalborczyk, please take a look.

[lukastaegert]

Closing this in favor of #4501, which should solve the same issue but I think is a more general solution.