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 and enhance resolver.getNpmPaths #52

Closed
StefanoMagrassi opened this issue Dec 17, 2015 · 5 comments
Closed

Fix and enhance resolver.getNpmPaths #52

StefanoMagrassi opened this issue Dec 17, 2015 · 5 comments
Labels

Comments

@StefanoMagrassi
Copy link

The resolver.getNpmPaths() method makes some basically wrong assumptions.

The entire block from line 123 to line 142 (comments included) it's too based on the experience/environment of the developer who implemented the code.

Npm allows to configure a custom path for global node_modules directory and the default one can also be different from one platform to another.

This can lead to unexpected behaviour: I configured a custom npm prefix, installed the generator-webapp globally without problems and now I can't find it in the list of available generators.

It would be better to take advantage of npm api to programmatically get the global node_modules path, in order to ensure a more "environment-agnostic" way to search for generators.

Check out these resources:

The programmatic api seems to be supported from npm version 2.0.1 (the oldest one on Github).


This is a simple snippet that shows how to get it with npm api:

var npm = require('npm');

function npmGlobalDir(cb) {
  npm.load({}, function(err) {
    if (err) {
      throw err;
    }

    cb(npm.globalDir);
  });
}

Obviously, the getNpmPaths() should no longer be a synchronous method that returns a value, but one which expects a callback.
Also the lookup() method should change accordingly.

@SBoudrias
Copy link
Member

Did you run yo doctor?

My guess is that your issue is another one.

I've spent a LOT of time trying to get something working with NPM. It was either utterly slow of very brittle.

That being said, the fallback we're using might break with the new npm 3 flat layout given some different configuration. FWIW, it haven't broke for me, and I also use a custom npm prefix.

Either way, if you feel the npm programmatic api is now stable enough to be used, feel free to send a PR. Any improvement will be gladly accepted.

@StefanoMagrassi
Copy link
Author

The npm README says:

The semantic version of npm refers to the CLI itself, rather than the underlying API. The internal API is not guaranteed to remain stable even when npm's version indicates no breaking changes have been made according to semver.

so, the programmatic solution isn't totally safe.
But this specific piece of code didn't change since version 1.x (as I can suppose watching the code in 1.0 branch on npm repo).

I will make some test in order to understand if it's a problem due to my environment and if the "api way" could be a good solution.

@StefanoMagrassi
Copy link
Author

I've run yo doctor:

✖ NODE_PATH matches the npm root
npm global root value is not in your NODE_PATH

Ok, I think it would be better if I add the global node_modules path to the NODE_PATH env var, even if it's not mandatory.

But I read the code in yo doctor node_path rule and there it's called the npm cli through childProcess.exec.

I think that we could also use this approach if we want to avoid the npm programmatic api instability.

@SBoudrias what do you think about it?

@SBoudrias
Copy link
Member

@StefanoMagrassi childProcess.exec is unfortunately very very slow.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2020

This issue is stale because it has been open 15 days with no activity. Remove stale label or comment or this will be closed in 5 days

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

No branches or pull requests

2 participants