module: test for directories, fixes require with .. #58

robertkowalski · 2014-12-04T06:55:33Z

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 · 2014-12-04T07:37:50Z

lib/module.js

@@ -169,23 +169,24 @@ Module._findPath = function(request, paths) {
  // For each path
  for (var i = 0, PL = paths.length; i < PL; i++) {
    var basePath = path.resolve(paths[i], request);
+    var stats = statPath(basePath);


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?

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

bnoordhuis · 2014-12-04T21:28:29Z

test/fixtures/json-with-directory-name-module/module-stub/test/tap/test.js

@@ -0,0 +1 @@
+module.exports = require('../..');


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 · 2014-12-04T21:29:33Z

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

rvagg · 2014-12-04T23:46:31Z

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 · 2014-12-05T00:04:18Z

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.

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.

robertkowalski · 2014-12-05T23:40:48Z

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

robertkowalski · 2014-12-05T23:43:13Z

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 · 2014-12-06T08:23:35Z

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 · 2014-12-06T15:27:16Z

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)

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. PR-URL: #58 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-by: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

bnoordhuis · 2014-12-07T07:20:41Z

Thanks Robert, landed in 36777d2.

Original commit message: Merged: [liftoff][arm] Release temp registers after use The {ParallelRegisterMove} at the end of {AtomicLoad} might need a temporary scratch register for spilling values to the stack. Make sure that one is available by giving up the scratch register used for the address of the atomic access. TBR=ahaas@chromium.org (cherry picked from commit 63166010061d2af4fef6a713d448ebf074a9d2cb) (cherry picked from commit 953f7a9dcb1425616e3be67fdfe6ef8d820f0daa) Bug: chromium:1153442 Change-Id: Ie312b37857e226058581b300b5adb1f14476c155 No-Try: true No-Presubmit: true No-Tree-Checks: true Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2584959 Reviewed-by: Clemens Backes <clemensb@chromium.org> Commit-Queue: Clemens Backes <clemensb@chromium.org> Cr-Original-Commit-Position: refs/branch-heads/8.7@{nodejs#60} Cr-Original-Branched-From: 0d81cd72688512abcbe1601015baee390c484a6a-refs/heads/8.7.220@{#1} Cr-Original-Branched-From: 942c2ef85caef00fcf02517d049f05e9a3d4b440-refs/heads/master@{#70196} Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2656263 Reviewed-by: Victor-Gabriel Savu <vsavu@google.com> Commit-Queue: Artem Sumaneev <asumaneev@google.com> Cr-Commit-Position: refs/branch-heads/8.6@{nodejs#58} Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1} Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472} Refs: v8/v8@5c6c99a

Original commit message: Merged: [liftoff][arm] Release temp registers after use The {ParallelRegisterMove} at the end of {AtomicLoad} might need a temporary scratch register for spilling values to the stack. Make sure that one is available by giving up the scratch register used for the address of the atomic access. TBR=ahaas@chromium.org (cherry picked from commit 63166010061d2af4fef6a713d448ebf074a9d2cb) (cherry picked from commit 953f7a9dcb1425616e3be67fdfe6ef8d820f0daa) Bug: chromium:1153442 Change-Id: Ie312b37857e226058581b300b5adb1f14476c155 No-Try: true No-Presubmit: true No-Tree-Checks: true Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2584959 Reviewed-by: Clemens Backes <clemensb@chromium.org> Commit-Queue: Clemens Backes <clemensb@chromium.org> Cr-Original-Commit-Position: refs/branch-heads/8.7@{#60} Cr-Original-Branched-From: 0d81cd72688512abcbe1601015baee390c484a6a-refs/heads/8.7.220@{#1} Cr-Original-Branched-From: 942c2ef85caef00fcf02517d049f05e9a3d4b440-refs/heads/master@{#70196} Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2656263 Reviewed-by: Victor-Gabriel Savu <vsavu@google.com> Commit-Queue: Artem Sumaneev <asumaneev@google.com> Cr-Commit-Position: refs/branch-heads/8.6@{#58} Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1} Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472} Refs: v8/v8@5c6c99a PR-URL: #38275 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Shelley Vohr <codebytere@gmail.com>

rvagg reviewed Dec 4, 2014
View reviewed changes

rvagg force-pushed the v0.12 branch 4 times, most recently from d7e65ff to 185d11c Compare December 4, 2014 10:21

bnoordhuis reviewed Dec 4, 2014
View reviewed changes

bnoordhuis closed this Dec 7, 2014

rvagg mentioned this pull request Jan 3, 2015

governance: Add new Collaborators #234

Closed

joyeecheung mentioned this pull request Feb 20, 2020

repl: printing big strings results in OOM #25478

Closed

wmertens mentioned this pull request Jun 19, 2020

uv__io_stop: Assertion `loop->watchers[w->fd] == w' failed. #33966

Closed

silverwind mentioned this pull request Aug 20, 2020

https/http2: random crashes on macOS with simple server #34854

Closed

solebox mentioned this pull request May 13, 2022

[Snyk] Fix for 1 vulnerabilities solebox/node#29

Open

enterstudio mentioned this pull request May 14, 2022

[Snyk] Fix for 1 vulnerabilities enterstudio/node#28

Open

UbuntuEvangelist mentioned this pull request May 14, 2022

[Snyk] Fix for 1 vulnerabilities UbuntuEvangelist/node#22

Open

s4sc mentioned this pull request May 14, 2022

[Snyk] Fix for 1 vulnerabilities s4sc/node#28

Open

duecorda mentioned this pull request Jun 19, 2022

for loop, inside async setter doesn't wait all Promise. nodejs/help#3898

Closed

caijw mentioned this pull request Jul 20, 2022

cross compiling for android arm fail. #43906

Closed

markcellus mentioned this pull request Oct 27, 2022

URL.href adds a trailing slash at end, but browsers don't #45216

Closed

nitram-work mentioned this pull request Nov 3, 2022

Zlib inflate() holds and reuses buffer until program exit, is never freed limiting memory for other uses. #45303

Open

gnarea mentioned this pull request Nov 3, 2022

RSA key in JWK format can't be imported with createPublicKey() #45304

Closed

vvvbbbcz mentioned this pull request Nov 5, 2022

Can't run npm on node 18.12.1 on Linux #45329

Closed

Nugine mentioned this pull request Nov 15, 2022

Executing unoptimized wasm file causes OOM #45480

Closed

pdpaulson mentioned this pull request Nov 16, 2022

Cannot pass script file on embedtest command line #45484

Closed

marcossv9 mentioned this pull request Nov 17, 2022

nodejs@16.18.1-deb-1nodesource1 package contains security issues #45499

Closed

snyk-bot mentioned this pull request Jan 8, 2023

[Snyk] Upgrade @humanwhocodes/config-array from 0.9.5 to 0.11.8 AshdonG/node#3

Open

nickfnblum mentioned this pull request Nov 30, 2023

[Snyk] Fix for 1 vulnerabilities nickfnblum/node#12

Open

xphillyx mentioned this pull request Nov 30, 2023

[Snyk] Fix for 1 vulnerabilities xphillyx/node#27

Open

terrorizer1980 mentioned this pull request Dec 1, 2023

[Snyk] Fix for 3 vulnerabilities terrorizer1980/node-1#15

Open

kaocher82 mentioned this pull request Dec 1, 2023

[Snyk] Fix for 1 vulnerabilities Xtuden-com/node#39

Open

terrorizer1980 mentioned this pull request Dec 1, 2023

[Snyk] Fix for 1 vulnerabilities Exhorder6/node#24

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: test for directories, fixes require with .. #58

module: test for directories, fixes require with .. #58

robertkowalski commented Dec 4, 2014

rvagg Dec 4, 2014

robertkowalski Dec 4, 2014

bnoordhuis Dec 4, 2014

bnoordhuis commented Dec 4, 2014

rvagg commented Dec 4, 2014

rlidwka commented Dec 5, 2014

robertkowalski commented Dec 5, 2014

robertkowalski commented Dec 5, 2014

chrisdickinson commented Dec 6, 2014

robertkowalski commented Dec 6, 2014

bnoordhuis commented Dec 7, 2014

module: test for directories, fixes require with .. #58

module: test for directories, fixes require with .. #58

Conversation

robertkowalski commented Dec 4, 2014

rvagg Dec 4, 2014

Choose a reason for hiding this comment

robertkowalski Dec 4, 2014

Choose a reason for hiding this comment

bnoordhuis Dec 4, 2014

Choose a reason for hiding this comment

bnoordhuis commented Dec 4, 2014

rvagg commented Dec 4, 2014

rlidwka commented Dec 5, 2014

robertkowalski commented Dec 5, 2014

robertkowalski commented Dec 5, 2014

chrisdickinson commented Dec 6, 2014

robertkowalski commented Dec 6, 2014

bnoordhuis commented Dec 7, 2014