Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

module: fix resolution of filename with trailing slash #6215

Closed
wants to merge 1 commit into from
Closed

module: fix resolution of filename with trailing slash #6215

wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Apr 15, 2016 •
edited by Fishrock123
Loading

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

module

Description of change

A recent optimization of module loading performance [1] forgot to check that
extensions were set in a certain code path.

[1] ae18bbe

Fixes: #6214

@targos targos added the module Issues and PRs related to the module subsystem. label Apr 15, 2016
@targos
Copy link
Member Author

targos commented Apr 15, 2016

I added a test in a separate commit but I'm not sure it's very useful. If the fixtures are removed by accident, the test would still pass.

@jasnell
Copy link
Member

jasnell commented Apr 15, 2016

LGTM

@jasnell jasnell mentioned this pull request Apr 15, 2016
Closed
@Salakar
Copy link

Salakar commented Apr 15, 2016 •
edited
Loading

Does this resolve issues with require('../') ? Looks like it does

See Salakar/mocha@7c07b67

Salakar referenced this pull request in Salakar/mocha Apr 15, 2016
@Salakar
Change mocha require path
7c07b67 
Currently in node 6 (RC2) the `require('../')` breaks with the below error:

```
module.js:128
  for (var i = 0; i < exts.length; i++) {
                          ^

TypeError: Cannot read property 'length' of undefined
    at tryExtensions (module.js:128:27)
    at tryPackage (module.js:107:10)
    at Function.Module._findPath (module.js:182:18)
    at Function.Module._resolveFilename (module.js:434:25)
    at Function.Module._load (module.js:384:25)
    at Module.require (module.js:464:17)
    at require (internal/module.js:16:19)
    at Object.<anonymous> (/Users/XXXX/Documents/Personal/Projects/XXXXX/node_modules/mocha/bin/_mocha:12:11)
    at Module._compile (module.js:539:32)
    at Object.Module._extensions..js (module.js:548:10)
```

Changing to `require('./../lib/mocha')` resolves the issue.
@mscdex
Copy link
Contributor

mscdex commented Apr 15, 2016

LGTM if CI is ok: https://ci.nodejs.org/job/node-test-pull-request/2275/

@targos targos added this to the 6.0.0 milestone Apr 16, 2016
@Trott
Copy link
Member

Trott commented Apr 16, 2016

I added a test in a separate commit but I'm not sure it's very useful. If the fixtures are removed by accident, the test would still pass.

If you want to guard against that, perhaps you can either:

  • check for the fixture file's existence as part of the test
  • use the tmp dir instead of fixtures and create the file as part of the test

@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

Is this ready to land?

@MylesBorins
Copy link
Contributor

citgm: https://ci.nodejs.org/job/thealphanerd-smoker/200/

(running because this touches module)

@targos
module: fix resolution of filename with trailing slash
1f6816b 
A recent optimization of module loading performance [1] forgot to check that
extensions were set in a certain code path.

[1] nodejs@ae18bbe

Fixes: nodejs#6214
@targos
Copy link
Member Author

targos commented Apr 19, 2016

I consolidated the test.
CI: https://ci.nodejs.org/job/node-test-commit/2968/

@jasnell
Copy link
Member

jasnell commented Apr 20, 2016

CI is green but there are a couple of CITGM failures. It's not clear if they're related but I doubt it. The failure here looks like it's related to the recent realpath changes.

@MylesBorins
Copy link
Contributor

There has been a flury of activity on master. I'm running citgm on the head of master to see if these breakages are related to something else

@jasnell
Copy link
Member

jasnell commented Apr 20, 2016

New CI: https://ci.nodejs.org/job/node-test-pull-request/2343/

@MylesBorins
Copy link
Contributor

So funny enough... without out this fix master is even more broken.... lol

@jasnell
Copy link
Member

jasnell commented Apr 20, 2016

Indeed. if the new CI comes up green I'll be getting this landed.

jasnell pushed a commit that referenced this pull request Apr 20, 2016
@targos @jasnell
module: fix resolution of filename with trailing slash
75487f0 
A recent optimization of module loading performance [1] forgot to check that
extensions were set in a certain code path.

[1] ae18bbe

Fixes: #6214
PR-URL: #6215
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell
Copy link
Member

jasnell commented Apr 20, 2016

Landed in 75487f0

@jasnell jasnell closed this Apr 20, 2016
@targos targos deleted the fix-6214 branch April 20, 2016 17:40
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
@targos @joelostrowski
module: fix resolution of filename with trailing slash
aeb613f 
A recent optimization of module loading performance [1] forgot to check that
extensions were set in a certain code path.

[1] nodejs@ae18bbe

Fixes: nodejs#6214
PR-URL: nodejs#6215
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
@targos @jasnell
module: fix resolution of filename with trailing slash
f3d9de7 
A recent optimization of module loading performance [1] forgot to check that
extensions were set in a certain code path.

[1] ae18bbe

Fixes: #6214
PR-URL: #6215
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
module Issues and PRs related to the module subsystem.
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

module resolution can fail with trailing slash
6 participants
@targos @jasnell @Salakar @mscdex @Trott @MylesBorins