module: test for directories, fixes require with ..

[robertkowalski]

Given my home-directory is /Users/rocko - and I have a file named
npm.json in it and also a repository with name npm, which is a
folder for the node-module.

When try to require the /Users/rocko/npm/index.js two direcotry
levels down in the npm folder (e.g. /Users/rocko/npm/test/tap)
with require("../../") node will load /Users/rocko/npm/index.json.

When I use require("../..") node will load /Users/rocko/npm.json
which is fixed by this commit.

Original PR: nodejs/node-v0.x-archive#7094

[rvagg]

wondering if I'm missing something, why is this done up here and not closer to where it's used, potentially skipping when trailingSlash==true?

[robertkowalski]

good point! no real reason for that, will fix that!

[bnoordhuis]

It would be good to have a second test that checks that ../../ has the same behavior.

Suggestion: change the file path to a more neutral one, e.g. test/fixtures/json-with-directory-name-module/module-stub/one/two/three.js. The tap/ directory suggests that tap is somehow involved when it's not.

[bnoordhuis]

Left two comments but apart from that LGTM. Would be good to get more eyeballs.

[rvagg]

lgtm, having reviewed the original discussion @ joyent/node I'm a big +1 on consistency and think holding off for some mythical future total rework of the module loader system is a bad idea, let's just land this and think about the future when the future happens

[rlidwka]

Require subsystem is locked in node.js. Are we changing it here or not?

If we are, I want to ask require('.') to be handled the same as require('./'), because the current behavior of require('.') is useless.

[robertkowalski]

thanks for the feedback. I added the additional test and changed the directory of the fixture.

[robertkowalski]

some background info: my last status from joyent was that they want to merge it in an unstable version - they also were quite afraid that it could slow down application startup so i had to create all of these benchmarks in the original pr

[chrisdickinson]

Oh wow, that's weird behavior -- good catch & thanks for fixing it. I'm +1 on changing this behavior, it seems very much like a bug given the docs.

[robertkowalski]

yes, it is a bug, as you can't reach a directory next to a{js,json}-file which has the same name - with the patch you are able to reach both the file and the directory (by adding the fileending to the require-argument to take the file - or leaving it off to take the directory)

[bnoordhuis]

Thanks Robert, landed in 36777d2.