fix(global-prettier-resolution): add fallback for NVM users

[kachkaev]

Closes #448

When Prettier is not found in the current project, the extension first attempts to find a global instance of it and finally falls back to the built-in instance. The second step is important for those who want to use Prettier with plugins, but does not want to see local node_modules in each project just because of it. Searching for global Prettier was added in #397 by me; we use this feature in litvis.

Recently, my colleague @jwoLondon discovered that global Prettier does not get resolved when Node and NPM are installed via NVM. This setup method is pretty prevalent and is especially helpful when a user does not have admin rights (e.g. on managed company laptops and in education environments such as university labs).

We investigated the problem and discovered that it comes from global-modules, which does not work as reliably as the npm get prefix command. Related issues:

• does not work inside Atom (packages) jonschlinkert/global-prefix#19
• Does not return the correct absolute path jonschlinkert/global-prefix#20
• Discussion to improve reliability on Windows jonschlinkert/global-prefix#21

This PR introduces a fallback search mechanism for global node modules, which should at least help NVM users, but maybe solves the issue in other edge cases too. I tested it by creating a new macOS user, installing NVM and then running npm install --global prettier prettier-plugin-elm. Debug info no longer shows prettier: bundled – it is now /Users/tester/.nvm/versions/node/v12.11.1/lib/node_modules/prettier/index.js. Global plugins work! 🎉

I decided to add the fallback search method instead of replacing the existing one to make sure the fix is non-breaking. Hope it can land the next patch release soon and it is much needed at City, University of London this term!

[codecov-io]

Codecov Report

Merging #516 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #516   +/-   ##
=====================================
  Coverage       0%     0%           
=====================================
  Files           1      1           
  Lines          61     61           
  Branches        7      7           
=====================================
  Misses         54     54           
  Partials        7      7

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbce7e8...3b2bc8b. Read the comment docs.

[robwise]

Thanks for figuring all of this out! I'm just one guy and there's a lot of environment-specific problems that I just don't come across because I only have my and my company's team as "test subjects".

I'd be happy to cut a new release just specifically for this once we merge.

I did have a comment about possibly swallowing too much with that try/catch? (See comment on specific line).

Also, would you mind changing the commit message to follow the conventional changelog pattern? We're using an automated changelog script on this repo, so unless the commit message follows that convention, the changelog won't pick it up.

I think it's something like: fix(global-prettier-resolution): add fallback for NVM users (or whatever, but it's like that format).

[robwise]

Which line specifically is it that can throw an exception and why? From looking at execa, it looks like it can throw errors if you call a bad command, for example. However, we probably don't want to swallow errors in that case like this?

Do we expect that sometimes the user won't have npm on the path or something and that's why we have the catch? If so, we should do a pre-emptive guard for that by executing which?

For example, here is roughly the same approach but no try/catch so that we don't swallow unexpected errors:

const npmIsAvailable = () => !_.isEmpty(execaSync("which", ["npm"]).stdout);

const npmPrefix = () => execaSync("npm", ["get", "prefix"]).stdout;

const resolveGlobalNodeModulesFromPrefix = prefix =>
  process.platform === "win32" ||
  process.env.OSTYPE === "msys" ||
  process.env.OSTYPE === "cygwin"
    ? path.resolve(prefix, "node_modules")
    : path.resolve(prefix, "lib/node_modules");

const resolveGlobalNodeModulesWithNpm = () => _.flow(npmPrefix, resolveGlobalNodeModulesFromPrefix);

const getFallbackGlobalModulesPath = _.memoize(_.cond([[npmIsAvailable, resolveGlobalNodeModulesWithNpm]]));

[kachkaev]

Which line specifically is it that can throw an exception and why?

I'm generally trying to guard the execa call just in case, since the signature of that function does allow for an exception to be thrown. I can't think of a concrete example, but it still feels safer with try / catch. I would prefer this solution over calling which because which may (?) not be available on Windows and also because it's another sync CLI call that takes time.

Your solution implies calling execaSync every time, even when fallback is not needed. This is what I was tryign to avoid by implementing getFallbackGlobalModulesPath the way I did. I'm totally happy to copy-paste your code though because I don't think that an extra CLI call is that expensive. WDYT?

[robwise]

Well, the whole thing does get memoized at the end in the code sample I put so there should only be those two calls to execa and then that's it.

But, the fact that which may not be present in Windows is concerning. I wasn't thinking about that. Let's merge for now and then we can always address later if we find this becomes a problem (which it probably won't).

[robwise]

Can we use lodash/fp here? Shouldn't actually affect your code but since we're using it everywhere else, it might be confusing if someone else comes along and tries to add some additional lodash imports for something they're doing.

[kachkaev]

Sure thing, I'll update the PR with lodash/fp. I did not notice this was the main flavour of the lib used in the repo.

[kachkaev]

Thank you for your feedback @robwise! I'll be happy to do a round of changes to this PR after hearing back from you 😉

[robwise]

LGTM

[robwise]

Oh crap we changed the PR title not the commit, so it's not going to show up in the changelog. But I will cut a release 0.57.3 now.

[kachkaev]

Many thanks for cutting the release so quickly @robwise! All works 🎉

No worries at all about the changelog – sorry for not fully understanding what you meant. I thought you'd squash and merge, so the commit message would contain the title of the PR.

[robwise]

Oh yeah I'm in the habit of just doing the regular merge commits (personal preference, I like seeing the PRs in the git history). I'm glad this is working for the rest of your team now!