nodejs / node-v0.x-archive

Moved to https://github.com/nodejs/node
34.41k stars 7.31k forks source link

require(filename) case sensitivity but windows filenames are case insensitivity #6000

Closed ghost closed 11 years ago

ghost commented 11 years ago

in my "file1.js" i have use the require like this: require('test.js');

in my "file2.js" i have use the require like this: require('Test.js');

Test.js and test.js files are identical on windows. But require cached the files to two different modules.

bnoordhuis commented 11 years ago

This has been brought up a few times before but we don't consider it a bug. On Windows the paths may be identical but on other platforms they definitely are not.

ghost commented 11 years ago

At least you should throw a warning about it. This is clearly a bug for windows. If you don't support windows, it's ok. But you claim that node supports windows, then you should at least acknowledge this bug.

bnoordhuis commented 11 years ago

IIf you have a strong opinion on the subject, please follow up with a pull request and we'll take it from there.

Don't get your hopes up though; there must be at least several 100k people developing on Windows and the topic of case sensitivity hardly ever seems to come up. Everything I've seen so far suggests it's mostly a non-issue in every day use.

ghost commented 11 years ago

Thanks for the advices.

I've looked for the underlying problem and found that fs.realpath function doesn't try to correct case. This leads node.js to cache the same module twice (in my example). Is this behaviour is intended? Because PHP's realpath function corrects cases too.

As far as i see, fixing this issue won't break anything. I'll try to make a pull request about this issue on my free time.

bnoordhuis commented 11 years ago

A pull request that attempts to fix up file paths will almost certainly be rejected. Printing a warning however might be acceptable.

isaacs commented 11 years ago

I'm firmly against changes to our case sensitivity handling in the module system.

  1. It is not a problem in everyday use.
  2. Even on Unix systems, you cannot know whether the fs is case sensitive by the os/arch information, so it will definitely cause bugs (or at least, strange behavior) in some cases.

This area of node is frozen for a reason. Please help us focus attention on the areas of node that are in need of improvement. The module system is finished.

mathiasbynens commented 11 years ago

It is not a problem in everyday use.

Maybe not, but it would be nice to be able to do stuff like this on all platforms:

readable way of loading polyfills

The same thing goes for packages like lodash.forOwnRight which are named after the property name in the Lo-Dash library. require('lodash.forOwnRight') only works reliably on case-insensitive file systems, but require('lodash.forownright') (which works everywhere) looks weird and inconsistent.

Even on Unix systems, you cannot know whether the fs is case sensitive by the os/arch information, so it will definitely cause bugs (or at least, strange behavior) in some cases.

The file path normalization would occur only if the original path doesn’t exist. In that case, how would it introduce bugs or strange behavior? All code that works now would still work, on all platforms. And code that works only on case-insensitive file systems today would then work everywhere. What am I missing?

bnoordhuis commented 11 years ago

require('lodash.forOwnRight') only works reliably on case-insensitive file systems, but require('lodash.forownright') (which works everywhere) looks weird and inconsistent.

Why don't you call the file lodash.forOwnRight.js then? Problem solved, right?

dominictarr commented 11 years ago

just make it: lodash.for-own-right

Generally, camelCase -> camel-case in the module name. and you could argue (although, it would be a bikeshed) that dashed is more readable,

mathiasbynens commented 11 years ago

Why don't you call the file lodash.forOwnRight.js then? Problem solved, right?

Because it’s an npm package (require(packageName), and npm doesn’t allow uppercase characters in package names, because require(filename) in Node.js is inconsistent in the way it handles lettercasing (i.e. this bug ticket). Catch-22

just make it: lodash.for-own-right

The reason why it would be nice to support e.g. require('lodash.forOwnRight') on all platforms is that it’s the same name as the property name that maps to this function, i.e. when using a complete Lo-Dash build, you can use this function as follows: var forOwnRight = _.forOwnRight. With separate modules for each function, this could then become var forOwnRight = require('lodash.forOwnRight'). If Node.js implemented file path normalization so require(foo) is case insensitive on all platforms, there could be a 1:1 mapping between the method name and the package name.

This would also be useful for polyfills: imagine loading a String.prototype.codePointAt polyfill using the following code:

require('String.prototype.codePointAt');

There’s nothing more readable than that, right?

dominictarr commented 11 years ago

Now, I feel your pain, because I've made similar suggestions before, and although, there may be a entirely reasonable argument for this, that while it would be nice to have, there are just too many little nice to haves to add them to node. Especially when it's a change to something that is already working well, and hasn't changed in a long time.

There are many many other little things that could be tweaked to make node just a little bit nicer, but because there are so many little things, if node took all the little changes, it would be constantly shifting, and things would probably be constantly breaking because of what are really quite unimportant things.

Not to mention the work load on the core team.

We are all way better off to be stable, and only change the big problems, and performance problems.

But, believe me, if I ever get my time machine working properly, the FIRST thing I will do (if that makes sense) will be to go back and make sure that @isaacs et al get these little things right in the first place :)

dominictarr commented 11 years ago

Although, I might have a hard time... travelling back to 2009 and telling ry that his posix polyfil will one day run as well on windows as on linux, and so that he should make the module system case insensitive.

mathiasbynens commented 11 years ago

@dominictarr I hear ya, but this is just one change — one that would make Node.js more consistent across platforms. No one’s asking for a “huge shift” in how Node works, or to break back-compat in any way.

Another way to avoid the issue is to have npm allow uppercase characters in package names again.

jdalton commented 11 years ago

bnoordhuis: Why don't you call the file lodash.forOwnRight.js then? Problem solved, right?

mathiasbynens: Because it’s an npm package (require(packageName), and npm doesn’t allow uppercase characters in package names, because require(filename) in Node.js is inconsistent in the way it handles lettercasing (i.e. this bug ticket). Catch-22

It would be fantastic to be able to call the file lodash.forOwnRight.js or String.fromCodePoint.js but because of npm's restriction to all lowercase names it makes a no go. The issue for our use case would be solved if npm allowed names of any case and simply punted on attempts to register another package of the same name regardless of case instead of enforcing lower case names. Then devs could do require('String.fromCodePoint') across all platforms consistently.

dominictarr commented 11 years ago

Okay, well, all I can say is good luck.

Modules have been frozen for ages now, http://nodejs.org/api/modules.html#modules_modules and there have been many public statements that modules will not change.

Maybe Isaac is partial to chocolate, back rubs, or bags of money? Maybe a round the clock vigil outside his house?

The simplest is just to use dashes, then you have a 1:1 with camel case, and neither npm or node has to change.

medikoo commented 11 years ago

The reason why it would be nice to support e.g. require('lodash.forOwnRight') on all platforms is that it’s the same name as the property name that maps to this function,

Convention of lowercase, dashed filenames, was coined a long while ago in node, and I think over time it proved to be right choice. Maybe just give it a try to adapt it? In light of case insensitive file systems it's really reasonable

I maintain similar toolkit in which each method is maintained as individual module, and on my side such require looks as:

require('es5-ext/string/#/code-point-at');

I have no issues with that.

gurdiga commented 9 years ago

Hey guys,

Today I had got a shameful bug related to this line:

var Q = require('Q');

It’s clearly a typo. The tests passed on my local machine (OS X Yosemite), but when deployed to Heroku (Ubuntu 14) the app broke because the module name is “q” not “Q”.

I know that OS X is generally (?) case insensitive—which probably makes it not a node or npm issue—and I’m wondering how do you guys protect against this kind of mistakes.

Sorry if my question is off-topic. I’d be thankful for any idea or link.