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

Fix for Issue #39 regarding NODE_PATH #47

Closed
wants to merge 3 commits into from
Closed

Fix for Issue #39 regarding NODE_PATH #47

wants to merge 3 commits into from

Conversation

job13er
Copy link

@job13er job13er commented Jun 10, 2014

  • Added tests to test/node_path.js to ensure $NODE_PATH environment
    variable is respected when searching for a module.
  • Updated the node-modules-paths module to include the directories
    specified by process.env.NODE_PATH.

 * Added tests to `test/node_path.js` to ensure `$NODE_PATH` environment
variable is respected when searching for a module.
 * Updated the `node-modules-paths` module to include the directories
specified by `process.env.NODE_PATH`.
@job13er
Copy link
Author

job13er commented Jun 10, 2014

Fixes #39

@@ -1,7 +1,7 @@
{
"name" : "resolve",
"description" : "resolve like require.resolve() on behalf of files asynchronously and synchronously",
"version" : "0.7.1",
"version" : "0.7.2",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if you'd rather not include this in the PR. I'm not sure how you handle your releases.

@job13er
Copy link
Author

job13er commented Jun 10, 2014

Anyone know how to simulate the environment in the travis build system? I can't reproduce the build failure. I installed nvm and used the same commands as the travis build and got clean results

@job13er
Copy link
Author

job13er commented Jun 10, 2014

Nevermind, I figured it out, they have / in the NODE_PATH and so some other code is breaking once NODE_PATH is checked. I'll fix it.

@ghost
Copy link

ghost commented Jun 10, 2014

This is what opts.path is for. An implicit $NODE_PATH argument here seems very surprising.

@ghost ghost closed this Jun 10, 2014
@pikajude
Copy link

So if people are using NODE_PATH, and a project they want to install uses resolve.sync() instead of require(), they'll get an error? Is that intended behavior? Isn't this what NODE_PATH is for?

@job13er
Copy link
Author

job13er commented Sep 15, 2014

@joelteon

So if people are using NODE_PATH, and a project they want to install uses resolve.sync() instead of require(), they'll get an error?

Yes. We ran into that, which is why I tried to submit this fix. We weren't using node-resolve ourselves, but something we were consuming was, and since node-resolve didn't look in NODE_PATH we got an error.

Is that intended behavior?

Apparently, since my PR was dismissed. I guess it's intended or anyone who wants to use NODE_PATH to either

  1. Not use any projects which use node-resolve
  2. Use patched versions of every project that uses node-resolve to update their opts.path setting to account for NODE_PATH (as suggested above)
  3. Stop using NODE_PATH

We ended up going with option 3, since it appeared support for NODE_PATH wasn't very complete across libraries such as this one. And unlikely to change in the near future based on the response I got when I tried to add support for it here.

Isn't this what NODE_PATH is for?

We thought so. However, the latest docs do suggest not using NODE_PATH "You are highly encouraged to place your dependencies locally in node_modules folders. They will be loaded faster, and more reliably."

sternenseemann added a commit to sternenseemann/browserify-handbook that referenced this pull request Jul 21, 2020
browserify doesn't implicitly set opts.paths to NODE_PATH anymore
unlike stated previously in this documentation. For background see:

* browserify/resolve#39 (comment)
* browserify/resolve#47
* browserify/browserify#1626
sternenseemann added a commit to sternenseemann/browserify-handbook that referenced this pull request Jul 21, 2020
browserify doesn't implicitly set opts.paths to NODE_PATH anymore
unlike stated previously in this documentation. For background see:

* browserify/resolve#39 (comment)
* browserify/resolve#47
* browserify/browserify#1626
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants