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.

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

If $NODE_PATH ends with a colon, require will act unexpected #1487

Closed
wants to merge 1 commit into from

Conversation

chrisyip
Copy link
Contributor

Here is a repo that explains the difference: https://github.com/chrisyip/NODE_PATH_DEMO.

Using node v1.8.1
Current NODE_PATH is '/usr/local/lib/node_modules'
Executing 'node foo/bar.js', you should seeing an error...

In foo/bar.js
module.js:336
    throw err;
          ^
Error: Cannot find module 'index.js'
    at Function.Module._resolveFilename (module.js:334:15)
    at Function.Module._load (module.js:276:25)
    at Module.require (module.js:363:17)
    at require (module.js:382:17)
    at Object.<anonymous> (/Users/Chris/Workspace/node/NODE_PATH_DEMO/foo/bar.js:2:13)
    at Module._compile (module.js:428:26)
    at Object.Module._extensions..js (module.js:446:10)
    at Module.load (module.js:353:32)
    at Function.Module._load (module.js:308:12)
    at Function.Module.runMain (module.js:469:10)

Appeding a trailing colon to NODE_PATH
NODE_PATH now is '/usr/local/lib/node_modules:' <- notice the colon in the end
Executing 'node foo/bar.js'...

In foo/bar.js
index.js loaded
index.js: I should not be required

After separation, nodePath will become [ '/usr/local/lib/node_modules', '' ] (in the above case).

I know node should not take the responsibility for env variables, but since '' is meaningless, I suggest to remove '' from nodePath before using it.

@popomore
Copy link
Contributor

LGTM

PS: where is the CI?

@chrisyip
Copy link
Contributor Author

@popomore I added test cases, but I can't find CI guide on wiki page, and because there's no .travis.yml, don't know how to trigger CI. What should I do?

PS: should I use a new branch for PR?

@jbergstroem
Copy link
Member

@chrisyip for now, collaborators of io.js has access to start a CI run for you. Our ambition is to improve this so all PR's at some point gets testing. Btw -- when forking and doing PR's, it's always good to create a new branch for your work instead of using master.

CI run here: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/567/

@chrisyip
Copy link
Contributor Author

OK, I will create a new branch and make a PR again.

Sorry for the trouble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants