Replaced findup with resolve.

[callumacrae]

The actual code is much nicer.

This has the side effect that it drops support for NODE_PATH, because it isn't supported by resolve: substack/node-resolve#47. However, according to the Node.js docs, you're not supposed to use NODE_PATH anyway: https://nodejs.org/api/modules.html#modules_loading_from_the_global_folders

I can probably work around if you want NODE_PATH support or you don't want to break BC, but it would make the code quite a bit longer (it would involve a try-catch in a try-catch!)

closes #74

[jackfranklin]

WRT the removal of NODE_PATH support, that would be a breaking change - I've been thinking for a while of bumping this plugin up to V1 so if we were to do this it would be a good time to remove it and bump it up to V1.

I personally have not used NODE_PATH and agree with the article you linked to that it doesn't seem like a great idea. That said, @chmanie did the PR to add it and there may well be a use case I haven't thought of. Before we properly consider dropping it I'd be really keen to hear use cases for supporting it.

[callumacrae]

Sure.

I think something like this would work to keep NODE_PATH support (untested):

var src;
try {
  src = resolve.sync(name, { basedir: path.dirname(config) });
} catch (e) {
  if (e.message.indexOf('Cannot find module') === 0) {
    src = name;
  } else {
    throw e;
  }
}

return require(src);

However, it's not exactly very nice.

[chmanie]

I was using the NODE_PATH on my CI runner to have it use pre-installed packages. It was the only thing that would solve my problem of npm install hanging all the time and/or taking a really long time. I switched to another solution, but there may be people with a similar case.

[callumacrae]

What was the solution you switched to? Or do you mean you switched CIs?

[chmanie]

I checked in the dependencies as packages with https://github.com/JamieMason/shrinkpack

[callumacrae]

@jackfranklin: Your call. I think I add back support for NODE_PATH, but it wouldn't be pretty.

[jackfranklin]

Sorry for slow reply, let's ditch support for NODE_PATH. In which case, is this ready to merge?

[callumacrae]

No worries. Yep.

[cedx]

Very disappointed that NODE_PATH support was removed. Not everyone uses local node_modules (it's my case at work). Do you plan to restore it in the future?

[callumacrae]

@cedx What's the actual case for NODE_PATH support? Why not just use node_modules?

These are mostly for historic reasons. You are highly encouraged to place your dependencies locally in node_modules folders. They will be loaded faster, and more reliably.

https://nodejs.org/api/modules.html

[jackfranklin]

@cedx no one has come up yet with a really compelling argument for keeping support. If you can then we should certainly discuss :+1:

[cedx]

At my work, we have a very special setup based on a pool of Node.js modules. Instead of each project having its own node_modules folder, the projects are grouped by "category", and each category uses its own NODE_PATH to share the same modules. I know it's a little weird, but that's the real life :)

[callumacrae]

I feel sorry for every junior developer who joins your company!

I'm still not sure I understand your case, but if you send a PR adding support back, I don't see why @jackfranklin wouldn't merge it. It should just be a case of adding the test case removed in this PR back, and adding a require in a try-catch block.

[jackfranklin]

@cedx yup, what @callumacrae said. That's a horrible situation to be in though, sorry :P