windows: case-insensitive lookup in module cache #6774
Conversation
| @@ -271,8 +271,9 @@ Module._load = function(request, parent, isMain) { | |||
| } | |||
|
|
|||
| var filename = Module._resolveFilename(request, parent); | |||
| var cacheKey = (process.platform === 'win32') ? filename.toLowerCase() : filename; | |||
There was a problem hiding this comment.
80 column limit, also please remove parens before ternary.
|
Generally, LGTM. May I ask you to change |
|
I meant "fixes" as the third person of the verb "to fix" :) I can change it to "this fixes" |
|
It is not about grammar, mostly about style. We use "Fix" form, not "Fixes". |
|
Why should it be treated as case-insensitive? Does current implementation cause any real bugs in the code? What would happen if case-sensitive filesystem will be mounted on windows (no idea if it's possible)? |
The module cache key is the resolved filename which should be treated as case-insensitive on Windows. Fix test-module-loading.js on Windows.
|
Updated. |
|
I want to get feedback from @isaacs on this, but I am mostly ok with this. Is there a way to convince windows api calls to guarantee casing for us, or is it basically always our responsibility? |
|
Win32 APIs are all case-insensitive with regard to filenames. |
|
LOL on the "Fix" vs "Fixes" comments. |
|
+1 to get this fixed. Wrote about my experience here: http://jonmrdsj.wordpress.com/2014/04/06/node-js-double-inclusion-on-windows/ Basically, if you do require('./file'); require('./File'), it will load and execute the same file twice, which is not what I expected |
|
@tjfontaine any good reasons not to take this fix? |
|
Like #7880 says, still think nodejs should make the driver letter to be uppercase to paired with |
...and that's not even the biggest issue. Node internal APIs differ in which kind of drive letter to use in 0.11 which can cause a file to be re-evaluated twice even if you didn't specify any file twice in two different ways. This broke some Grunt public APIs for me; see #7806. |
|
@orangemocha ... is this still needed/relevant? |
|
@jasnell : still relevant, but since it's been a while I need to take a second look and make sure it's up to date and the best fix for the underlying issue. |
|
@orangemocha ... ok, it looks like @misterdjules submitted an alternate fix to the same problem... #8145 |
|
@jasnell : thanks for the link. I still think we need case normalization, which path.normalize doesn't do. While trying to load the same module with different casing would be most likely a user error, the module will still load, but twice. |
|
+1 .. definitely agree that the case normalization is important. |
|
Old topic but still a pain if you waste valuable hours of your life troubleshooting non-working singletons.. This is not just a Windows issue, Please have a look at this pull request #8145 that nails it. Note that there are two issues with require() case sensitivity, #8145 addresses the 2nd:
|
|
Please forgive me... I can't believe this actually is how node works on Windows. I just spent several hours debugging to find these issues have been written up for a couple years. I look at my require.cache and I have hundreds of Please just get someone at MSFT to make a PR.... (edited) |
The module cache key is the resolved filename which should
be treated as case-insensitive on Windows.
Fixes test-module-loading.js on Windows.