Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

path.join() changes casing of drive letter starting with 0.11.0 #7031

Closed
AlexZeitler opened this issue Feb 2, 2014 · 23 comments
Closed

path.join() changes casing of drive letter starting with 0.11.0 #7031

AlexZeitler opened this issue Feb 2, 2014 · 23 comments
Milestone

Comments

@AlexZeitler
Copy link

Running this piece of code on Windows and Node.js 0.10.25 results as shown below.

Code:

var path = require('path');
var join = path.join;
var root = __dirname;
var relativePath = '/relativepath'
var joinedPath = join(root, relativePath);
console.log("root:",__dirname);
console.log("relative path:",relativePath);
console.log("joined path:", joinedPath);

Result (Node.js 0.10.25 on Windows 7 Pro x64):

root: D:\Code\PDMLab\nodetests
relative path: /relativepath
joined path: D:\Code\PDMLab\nodetests\relativepath

Result running the same code on Node.js 0.11.0 and later on Windows 7 Pro x64:

root: D:\Code\PDMLab\nodetests
relative path: /relativepath
joined path: d:\Code\PDMLab\nodetests\relativepath

As you can see, the drive letter now is lower case.

This behavior breaks modules like koa-send when it comes to using joined path instances.

@larryb82
Copy link

a05f973

Changed the behaviour of normalize which is used by join: https://github.com/joyent/node/blob/b9bec2031e5f44f47cf01c3ec466ab8cddfa94f6/lib/path.js#L243

@dougwilson
Copy link
Member

Modules should be using path.normalize on both strings before trying to directly compare them. koa already fixed this in version 1.2.3 release yesterday.

@create-renegr
Copy link

I crushed into the same problem with another plugin I used.

Can you explain why the result of path.join is normalized by default, but the result of path.resolve is not?

I know nothing of the implementation details of the path library, so from a users perspective it seems to me kinda odd, to handle the results (within one library) differently.

Thanks for clarification!

@obastemur
Copy link

I have tried a similar .NET Framework methods to see whether they are updating the drive letter to a lower cap on Windows, but they didn't. I don't see a technical necessity behind this update.
notes on #5073

@create-renegr
Copy link

Thanks for your effort, but if this is an answer to my question, I did not grasp it. Sorry.

Why are the results handled differently?
Why does one function apply normalize to the result, whereas the other does not?
Is there anything wrong if every return value is being normalized by default to ensure consistency inside the library?

@obastemur
Copy link

Is there anything wrong if every return value is being normalized by default to ensure consistency inside the library?

I agree with you on consistency. IMHO I don't see the reason for a normalization that way, especially forcing drive letters to lowercase on Windows. Perhaps, there is a historical reason for that patch or simply not well considered on the bigger picture.

@fluidsonic
Copy link

This change also broke our code which _require_s some files using their full path:

require(path.join(__dirname, 'other.js'));

It works fine as long as the _require_d module wasn't already loaded using a relative path. But as soon as the newly loaded module _require_s other modules it loads all of them using a different case for the drive letter and thus all of them are loaded twice.

Simple test case:

console.log('Executing:', __filename);

var path = require('path');

require(__filename);
require(path.join(__filename));

The module _require_s itself which shouldn't do much. The first require uses the full path untouched and the second one uses path.join on the full path.

Expected output:

Executing: C:\some\long\path\test.js

Actual output:

Executing: C:\some\long\path\test.js
Executing: c:\some\long\path\test.js

The module is loaded twice. D'oh.

@migounette
Copy link

There is also a side effect with VM functions, in this case as you said you will obtain multiple copies in Module._cache, once you have joined or normalize the path via vm.runInNewContext(fs.readFileSync(... path.normalize(...), ..))

You will have multiple entries in Modules._cache for the same modules if you use relative paths, because the __dirname of the parent has now the drive letter in lowercase

My 2 cents

@zxc122333
Copy link

same issue. Some modules are loaded twice.

@pspaulding
Copy link

I also ran into an issue with some modules loading more than once, leading to all sorts of issues.

@zxc122333
Copy link

@tjfontaine ?

@bschuedzig
Copy link

same issue.. provokes all kind of funny situations when using things like require-all, etc.

We have currently a small workaround in place that enables us to continue for the moment.
https://gist.github.com/bschuedzig/9913901

(disclaimer: hackish temporary solution, here be dragons, don't use it, etc.)

@zxc122333
Copy link

@indutny ??

@indutny
Copy link
Member

indutny commented May 2, 2014

@zxc122333 ??

@zxc122333
Copy link

As fluidsonic said, the file will be loaded twice when I load it with

require(path.join(__dirname, 'other.js'));  // c:\path\other.js
require('./other.js') // C:\path\other.js

Is it a bug in node? Or I should use bschuedzig's patch?

p.s. v0.11.13 has the same problem
Sorry my poor English

@mrfabbri
Copy link

@create-renegr @obastemur I suppose, although documentation doesn't mention it, the toLowercase() change is to ensure a univocal representation of the given path on Windows (kind of getCanonicalPath() in Java, which, on the contrary, returns always an upper case drive letter).

What the documentation mentions is that for path.resolve([from ...], to) the resulting path is normalized, so the following should be always verified:

path.resolve(aPath) == path.normalize(path.resolve(aPath))

@obastemur
Copy link

From the Windows and it's frameworks point, there is no such functionality. (check .NET Framework's similar methods) All due respect to reasoning behind windows-drive-letter-case-update, IMHO changing one part of the entire framework to act like this has no sense. I had asked the reasoning to the owner of this commit months ago but no answer so far.

@bschuedzig
Copy link

AFAIK: windows itself ignores casing for file/directory access. It will create a file with the casing you desire, but you can access it after that no matter how the casing looks like. If you create a "TEST.TXT" it will show up with uppercases after that, but you can access it as "test.txt" or "tEsT.tXt" as well. The very core of windows does not care. Thus you won't be having any direct problems in either .NET or NodeJS.

IMHO the "only" problem is that NodeJS stores require'd files in a key/value store, where casing for the key matters. This makes total sense for *NIX platforms, as you might be able to create a "test" and "Test" module side by side and include them separately.

There are now many solutions on how to mitigate the problem, one would be at the very core (discussion about path.resolve/path.normalize), another one would be to have special handling for require on windows platforms. And I think, both can make sense (the latter one might be an easy fix for the time being, first one might be cleaner).

I just love the whole NodeJS ecosystem and all the hard work that went into that from a lot of people. It would be very sad to have many "false errors" reported on all different kind of modules from programmers on windows platforms who encounter weird results (for example require-all). The effects are very misleading (I remember the time it took me to understand the behavior).

mrfabbri added a commit to mrfabbri/node-static that referenced this issue Jun 18, 2014
- Calling resolve() doesn't fully normalize relative path on Windows (in Node v0.11.x, see
  nodejs/node-v0.x-archive#7031 ), i.e. doesn't lowercase drive letter, while normalize() does, which causes
  paths resulting from calling normalize() and join() (depending on normalize()) to differ and to
  break comparisons.

Fixes cloudhead#42.
mrfabbri added a commit to mrfabbri/node that referenced this issue Jun 18, 2014
Calling `resolve()` on Windows (in Node v0.11.0+) result in a path where drive
letter case isn't always lowercase (e.g. for a relative path like '.') while
`normalize()` always returns a path with a lower case drive letter.
This eventually breaks comparisons between results of `resolve()` and
`join()` (which relies on `normalize()`) for the same path.

Fixes nodejs#7031
lsegal added a commit to aws/aws-sdk-js that referenced this issue Jun 19, 2014
This commit fixes an issue where the SDK would not correctly
load with `require("aws-sdk")` on a Windows machine running Node.js
0.11.x. This error is caused by the following two behavioral changes
in Node.js:

* nodejs/node-v0.x-archive#6829
* nodejs/node-v0.x-archive#7031

In essence, Node.js `require()` calls return different objects for
differently cased filenames, even on case-insensitive file systems.
This, coupled with the fact that `path.join()` lowercases the drive
letter in the path, causes the SDK to try to load core.js two
separate times, creating two objects that do not have the
correct properties attached.

See #303 for more information.
@mgol
Copy link

mgol commented Jul 21, 2014

@bschuedzig

AFAIK: windows itself ignores casing for file/directory access.

That's only a half-truth. It's not a question of OS but file system. It so happens that the default NTFS configuration is to be case-insensitive and default ext4 & similar config is to be case-sensitive. OS X's HFS+ is case-insensitive be default as Windows.

But you can mount a case-sensitive NTFS etc. Node should still work in such scenario which means Node should store things in a case-sensitive way when needed.

@bschuedzig
Copy link

@mzgol, interesting.. never heard of that. thanks for clarifying 👍

misterdjules pushed a commit to misterdjules/node that referenced this issue Aug 12, 2014
When adding a new module to the cache, and also when querying the cache,
normalize the path so that modules don't get loaded more than once when
they are referenced by the same filename but with different case.

Fixes nodejs#7031.
misterdjules pushed a commit to misterdjules/node that referenced this issue Aug 12, 2014
When adding a new module to the cache, and also when querying the cache,
normalize the path so that modules don't get loaded more than once when
they are referenced by the same filename but with different case.

Fixes nodejs#7031.
misterdjules pushed a commit to misterdjules/node that referenced this issue Aug 12, 2014
When adding a new module to the cache, and also when querying the cache,
normalize the path so that modules don't get loaded more than once when
they are referenced by the same filename but with different case.

Fixes nodejs#7031.
misterdjules pushed a commit to misterdjules/node that referenced this issue Aug 12, 2014
When adding a new module to the cache, and also when querying the cache,
normalize the path so that modules don't get loaded more than once when
they are referenced by the same filename but with different case.

Fixes nodejs#7031.
misterdjules pushed a commit to misterdjules/node that referenced this issue Aug 14, 2014
When adding a new module to the cache, and also when querying the cache,
normalize the path so that modules don't get loaded more than once when:
- They are referenced by the same filename but with different case for
  the drive letter on Windows.
- They are referenced by two different paths that point to the same
  file.

Fixes nodejs#7031.
springmeyer pushed a commit to mapbox/node-pre-gyp that referenced this issue Nov 11, 2014
@UltCombo
Copy link

I'm having issues with [email protected] due to the inconsistent drive letter casing:

npm ERR! Windows_NT 6.1.7601
npm ERR! argv "C:\\nodist\\bin/v/0.11.14/node.exe" "C:\\nodist\\bin\\node_modules\\npm\\bin\\npm-cli.js" "rm" "gulp-6to5
"
npm ERR! node v0.11.14
npm ERR! npm  v2.1.14

npm ERR! c:\repos\6to5-node-skeleton\tpl\env\node\node_modules\gulp-6to5 is not a child of C:\repos\6to5-node-skeleton\tpl\env\node

I've also experienced issues with JSCS due to the drive letter casing inconsistency.

What is the plan to fix this? Revert the behavior back to preserving the drive letter? Make all path functions and process.cwd()/process.pwd()/__dirname/etc return normalized drive letters?

@othiym23
Copy link

The npm piece of this should be fixed in [email protected] (installable at present via npm install -g npm@next) – it was an npm bug that it wasn't using a function that properly handled path cases.

I agree that in general this behavior is confusing; either Node or libuv should deal with taking care of path normalization issues so application developers don't need to be aware of the intricacies of case-folding / case-insensitive filesystems and drive letters.

piscisaureus added a commit to piscisaureus/node that referenced this issue Dec 23, 2014
In general path functions don't change the case of a path. Making an
exception for windows drive letters violates the principle of least
surprise.

Changing the drive letter case has caused a lot of issues, including
nodejs#7031, nodejs#7806 and lots of bikeshedding about
whether uppercase is the right case or lowercase.

This effectively reverts nodejs/node-v0.x-archive@a05f973

PR-URL: nodejs/node#100
@qraynaud
Copy link

I'm not a node expert but I believe this casing issue on drive letters is really making lots of ink going down with none agreeing with each others. This probably means there is no good solution and thus I upvote reverting a05f973 like @piscisaureus seems to be advising.

Not touching the path is far from perfect but at least it won't change the problems we already had because of it (and have mostly found workarounds for) instead of creating new ones and possibly breaking a lot of npm packages we don't know about.

misterdjules pushed a commit to misterdjules/node that referenced this issue Jan 15, 2015
In general path functions don't change the case of a path. Making an
exception for windows drive letters violates the principle of least
surprise.

Changing the drive letter case has caused a lot of issues, including
nodejs#7031, nodejs#7806 and lots of bikeshedding about
whether uppercase is the right case or lowercase.

This effectively reverts nodejs/node-v0.x-archive@a05f973

PR-URL: nodejs/node#100
misterdjules pushed a commit to misterdjules/node that referenced this issue Jan 15, 2015
In general path functions don't change the case of a path. Making an
exception for windows drive letters violates the principle of least
surprise.

Changing the drive letter case has caused a lot of issues, including
nodejs#7031, nodejs#7806 and lots of bikeshedding about
whether uppercase is the right case or lowercase.

This effectively reverts nodejs/node-v0.x-archive@a05f973

Reviewed-by: Alexis Campailla <[email protected]>
Reviewed-by: Julien Gilli <[email protected]>
@Trott Trott closed this as completed Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests