Refactor Env#lookup

[SBoudrias]

Trimmed down version of #395 still relying on manual paths declaration after encountering too many bugs with NPM to allow using it in lieu of paths lookups.

[sindresorhus]

What kind of bugs did you encounter with npm? Is there something that could potentially be fixed in npm itself?

[SBoudrias]

Mostly this one: https://github.com/isaacs/npm/issues/4202

Thing is,

1. In perfect word, I'd use --parseable because it returns modules path. Though, it is bugged on global modules, so it won't work :(
2. So, I tried using --json. This is parseable, but less ideal as it only returns the module name, not the path. First I though this would've work using require.resolve, but Node.js can't resolve global dependencies paths using require.resolve if NODE_PATH global variable isn't set.

So, I kept path based lookup, but use NPM to find the global modules path here: https://github.com/yeoman/generator/pull/420/files#diff-49d286273f6324a7d8eb78b2924c40a1R55 - So, it's not the best, but it is already working way better for me!

[sindresorhus]

Hmm, ok. I think we can work around it for now, but would be nice to use npm fully in the future. Would you be able to do a pr on npm for isaacs/npm#4202?

Just a thought; Would it make sense for npm ls -g --json to include paths?

Can we get the npm global root, then set NODE_PATH, then do require.resolve?

[SBoudrias]

Can we get the npm global root, then set NODE_PATH, then do require.resolve?

Been there done that, wasn't working :(

I tried by doing it like so process.env.NODE_PATH = "npm/modules/root". Seemed like this env is internal to Node, so it wasn't shared with the spawned process. Also tried setting it by spawning command setting the Env, but Windows blocked that - haven't tried this one on Unix.

[addyosmani]

As per the meeting: we're happy to move forward with this implementation. Can we rebase and merge?

[SBoudrias]

Let's wait for #383 to be merged in before.

[sindresorhus]

opt = opt || {};

[sindresorhus]

Lgtm, but would still like to see some simple perf comparison pre/post of this.

[SBoudrias]

Before this fix, we had 68.54965612209382 ops/sec

Now merely 0.9368288955108388...

[SBoudrias]

I've been searching for other way to get global module root without requiring to spawn a command, and this is quite hard.

I tried using NPM from node, but that won't work as the global NPM create files in its modules that won't be read by any local NPM (and you can't require the global NPM modules if NODE_PATH isn't setup - and it isn't by default).

I really start to feel like we'll need to stick to guessing paths... Maybe we could check with if/else and base ourself on the NODE_PATH var if available, and otherwise fallback to best guess for each OS.

Any other ideas?

[addyosmani]

Maybe we could check with if/else and base ourself on the NODE_PATH var if available, and otherwise fallback to best guess for each OS.

Let's go ahead with this approach for now. Could you revise your PR to reflect this, @SBoudrias? :)

[SBoudrias]

Reverting the use of spawned npm root gave me those results on the benchmark suite. I'll update the PR shortly.

Before this fix, we had 68 ops/sec

With and without NODE_PATH, we got around 45

[addyosmani]

Woo!

[SBoudrias]

Had to fight Travis execution order to export a NODE_PATH to allow unit test to run on top the new logic. Finally got it!

BTW, previous benchmark assertion where wronged, I updated previous comment correctly. So we're losing a bit in performance, but way less than before.

[sindresorhus]

unused

[SBoudrias]

@sindresorhus Could you check and give a run to the sync version? For some reason I lost about 3ms in the benchmark, which is pretty weird considering...

[sindresorhus]

This is really strange. Looks like the sync one is considerably faster. I guess the perf issues are not in those codepaths.

Sync: Env#lookup 800.5611439952063
Async: Env#lookup 490.6814355911056

Btw, I ran the tests on this PR and one failed:

1) Environment Resolver #lookup register global generators:
     AssertionError: "undefined" == true
      at Context.<anonymous> (/Users/sindresorhus/dev/generator/test/env.resolver.js:48:14)
      at Test.Runnable.run (/Users/sindresorhus/dev/generator/node_modules/mocha/lib/runnable.js:211:32)
      at Runner.runTest (/Users/sindresorhus/dev/generator/node_modules/mocha/lib/runner.js:358:10)
      at /Users/sindresorhus/dev/generator/node_modules/mocha/lib/runner.js:404:12
      at next (/Users/sindresorhus/dev/generator/node_modules/mocha/lib/runner.js:284:14)
      at /Users/sindresorhus/dev/generator/node_modules/mocha/lib/runner.js:293:7
      at next (/Users/sindresorhus/dev/generator/node_modules/mocha/lib/runner.js:237:23)
      at Object._onImmediate (/Users/sindresorhus/dev/generator/node_modules/mocha/lib/runner.js:261:5)
      at processImmediate [as _immediateCallback] (timers.js:330:15)

[SBoudrias]

@sindresorhus That is possible as you're not running the tests from the global npm node_modules folder. So depending on your setup, it may not find the folder...

Can you give me where your npm modules are installed? (You can also try running the test suite setting up NODE_PATH env variable to the global module folder)

BTW, this fails now because there was no test coverage on the lookup method before this PR. (so it is probably not a bug with the current implementation)

[sindresorhus]

~/dev/generator/node_modules

[SBoudrias]

Yeah, that's it. As it is not a standard global module folder, the system miss it when it tries to guess the location of the global module folder.

[sindresorhus]

Right. Hmm, can you just ignore the test when the test env var is set? https://github.com/yeoman/yo/blob/master/cli.js#L122

[SBoudrias]

There, it skip if it is not defined.

[coveralls]

Coverage increased (+2.01%) when pulling db12d3aa13de66b164f80a1eea342b98ca0c6835 on SBoudrias:improve-lookup into f049269 on yeoman:master.

[SBoudrias]

Ok, reverted to the sync version.

FWIW, I ran the benchmark on my mac, and I got 738 ops/sec for the new version, and 120 ops/sec for the old version. (and 443 ops/sec for the async version)

Previously I had only runned those benchmark on my Windows box. I guess it was hitting some OS specific bottleneck (probably file/directory reading)...

Soooooooooo I guess we're OK performance wise.

@sindresorhus mind to run benchmark (node benchmarks/env.js) on your side and see if you have similar results?

[addyosmani]

(btw, you likely already know this but this PR will need to be rebased before we can merge :) 740 o/s on new version.

[sindresorhus]

@SBoudrias Env#lookup 802.8510136462345, feel free to merge when you fixed the merge conflict and squashed ;)

[coveralls]

Coverage increased (+2.0%) when pulling b06e170 on SBoudrias:improve-lookup into 86516bd on yeoman:master.

[SBoudrias]

Fixed generator-dummy to be a valid generator, and rebased. Merged away. (I squashed relevant commits together, but as there's a lot going in these, I prefer to keep each feature in its own commit so it is easier to follow on when needed).

[sindresorhus]

I squashed relevant commits together

Exactly what I wanted :)

[sindresorhus]

@SBoudrias this is really good! Thanks so much for your hard cleaning up the generator system :)