checks if babel is installed globally and displays correct cli message

[xtina-starr]

Q	A
Patch: Bug Fix?	yes
Major: Breaking Change?	no
Minor: New Feature?	no
Deprecations?	no
Spec Compliancy?	?
Tests Added/Pass?	no
Fixed Tickets	Fixes #5228
License	MIT
Doc PR	no
Dependency Changes	yes, just to the babel module.

I've added the is-global dependency to ensure this also works on windows. I check against isGlobal to determine if the process is executed globally or from the node module.

This was difficult to reproduce locally. I tried using npm link to test but that undermines the local check.

[mention-bot]

@xtina-starr, thanks for your PR! By analyzing the history of the files in this pull request, we identified @loganfsmyth and @hzoo to be potential reviewers.

[babel-bot]

Hey @xtina-starr! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

• Node vnode - linux

[loganfsmyth]

Interesting. We don't generally recommend people install the CLI globally. Perhaps we could only include the -g on the uninstall command, and recommend installing locally?

Edit: Swapped locally/globally :(

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@283d9cb). Click here to learn what that means.

@@           Coverage Diff            @@
##             master   #5258   +/-   ##
========================================
  Coverage          ?   89.2%           
========================================
  Files             ?     204           
  Lines             ?    9890           
  Branches          ?    2665           
========================================
  Hits              ?    8822           
  Misses            ?    1068           
  Partials          ?       0

Impacted Files	Coverage Δ
packages/babel/src/cli.js	0% <ø> (ø)

---

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 283d9cb...b6dc36f. Read the comment docs.

[xtina-starr]

@loganfsmyth thanks for the feedback. My latest commit edits the install command and fixes the lint errors.

[hzoo]

There is #5091 though @loganfsmyth? (invoke local command from global).

But yeah in the end babel doesn't do anything right now

[xtina-starr]

@hzoo I totally missed that the command contained babel versus babel-cli. Thanks for pointing that out. I've corrected it. Also in your comment were you suggesting to use process.cwd() to check if babel is being executed globally or locally?

[loganfsmyth]

I think there was a miscommunication, having this be babel and not babel-cli is on purpose, because babel is the package that contains the script that prints this warning, and we want to tell people to remove it.

[xtina-starr]

Ah, I see. Other than changing babel-cli back to babel is there any other feedback for this PR? Also is it okay just to add another commit to make this change or do you prefer I rebase to keep the commit history clean?

[loganfsmyth]

For PRs with a few work-in-progress commits like this, we'll probably github's squash-and-merge behavior, so you are free to push another commit and it'll all get squashed in the end.

I don't have any other comments, looks solid :)

[xtina-starr]

Sweet, thx!

[loganfsmyth]

Sooo, I got curious, and I'm not sure is-global actually works as advertised. Doing npm i babel in a local folder and then editing the CLI script to contain

#!/usr/bin/env node
console.log(require('is-global')())

then running node_modules/.bin/babel logs true for me even though it isn't global.

[xtina-starr]

Yikes, you are right! I refactored to use process.cwd() to determine if it was local versus global. I gave it a try on my local and it seems to produce the desired results. Do let me know if you have a better suggestion.

[loganfsmyth]

Getting an accurate true/false out of this is more of a pain than I expected!

I think we should avoid using cwd since that's not necessarily tied to the script location. Could we potentially compare __dirname and execPath?

[xtina-starr]

I'm not super familiar with the process obj, but it looks like execPath would be were the node process is executed rather than were the babel module was executed. An older article from the nodejs site states that modules installed locally are executed from ./node_modules/.bin/ so I'm checking for that in the process.env._ string.

In the global babel module
I executed this by using the babel command

console.log('from global');
console.log("__dirname:", __dirname);
console.log("process.env._:", process.env._);
console.log("execPath:", process.execPath);
console.log("cwd:", process.cwd());
console.log('isLocal:', process.env._.includes('/node_modules/.bin/babel'));

The above outputs:

from global
__dirname: /Users/artsy/.nvm/versions/node/v6.9.4/lib/node_modules/babel
process.env._: /Users/artsy/.nvm/versions/node/v6.9.4/bin/babel
execPath: /Users/artsy/.nvm/versions/node/v6.9.4/bin/node
cwd: /Users/artsy/.nvm/versions/node/v6.9.4/lib/node_modules/babel
isLocal: false

In the local babel module
I executed this by adding "babel": "babel" to scripts in the package.json file and running npm run babel in the command line

console.log("__dirname:", __dirname);
console.log("process.env._:", process.env._);
console.log("execPath:", process.execPath);
console.log("cwd:", process.cwd());
console.log('isLocal:', process.env._.includes('/node_modules/.bin/babel'));

outputs:

from node module
__dirname: /Users/artsy/Artsy/force/node_modules/babel
process.env._: /Users/artsy/Artsy/force/node_modules/.bin/babel
execPath: /Users/artsy/.nvm/versions/node/v6.9.4/bin/node
cwd: /Users/artsy/Artsy/force
isLocal: true

[loganfsmyth]

Thanks for the super thorough reply, I really appreciate the back and forth! I know it's making more work for you.

There are a few other concerns here

1. I think currently this check will fail on Windows because of / vs \ folder separators. That would be fixable.
2. This also assumes the user ran the binary like ./node_modules/.bin/babel but it is possible they ran it directly ./node_modules/babel/bin/babel.
3. If we do guess wrong about .bin/babel, this will default to -g, while it feels like we'd be better to default to leaving it off, since 2. in this list means that could happen depending on the execution command.

How about, as a rough guess at global, we check to see that the Babel executable is in the same folder as the Node command? It won't be perfect, but it would falling back to non-global when it's not extremely obvious that it is run globally. Say we did

var path = require('path');
var isGlobal = path.dirname(process.execPath) === path.dirname(process.env._ || '');

This will work in a standard Node environment on OSX and Linux as far as I know, though it'll fail on Windows. And if it guesses wrong, it'll just default to false so we leave out the -g.

[xtina-starr]

No worries about the back and forth. I'm learning in this process.

I'm not sure I fully understand the second concern, but I'm wondering if something like below would solve the windows concern?

const isLocalWin = process.env._.includes("\node_module\babel\.bin\babel");
const isLocalUnix = process.env._.includes("/node_module/.bin/babel");
const globalMessage = isLocalWin || isLocalUnix ? "" : " -g";

[loganfsmyth]

Yup! For .bin check you can use your suggestion and explicitly cover both cases, you can also do

path.sep + path.join('node_modules', '.bin', 'babel')

to build a path that adds the separators based on the current OS. However, there are there is another different that makes things on Windows more complicated.

I'll elaborate on my point in 2, since I didn't make it clear, and I think I had an error in it anyway. This will hopefully also help with the Windows vs Unix comparison.

When you install a module locally you get

node_modules/
  .bin/
    babel
  babel/
    cli.js

and when you use npm run, it will use the commands in .bin, so your check would work. However it's also possible that someone can run the command manually by doing ./node_modules/babel/cli.js, though in this specific case that is very unlikely.

Similarly, when installing globally, .bin does not exist, and the bin scripts are placed in a bin folder that is the same location that Node is stored in, at least in some cases.

If we only had to worry about Unix-like systems, your solution would be totally fine.

Unfortunately there are two pieces that will fail on Windows I believe:

1. I don't think process.env._ will exist, since that is a POSIX thing.
2. On Windows .bin/babel is a Batch .cmd file that explicitly executes node ../babel/cli.js as a command, meaning that as far as I know, Node itself won't know that it was executed as a .bin script or directly.

So currently I think 1. will cause your current PR to throw an exception due to it being undefined, and the proposal in your comment will have the same issue.

My proposal of

path.dirname(process.execPath) === path.dirname(process.env._ || '');

would detect when the script that was executed is in the same directory as node itself, which would only happen when installed globally. And if env._ does not exist, it will default to an empty string, which will also fail to match. So on Unixy stuff, it should detect the global install properly, and on Windows it will always return false.

I think this is a good compromise for now so that the detection works on Unix, and will default to non-global on Windows.

[xtina-starr]

This makes a lot sense and does seem like a reasonable compromise. Thanks for thoroughly walking through your explanation. I've just made the discussed changes.

[loganfsmyth]

Great, thanks for doing this. I pushed one extra commit into this PR because I realized there was an issue with the location of the file that was going to make things break on older Node versions.

Nothing for you to do, but if you're curious, essentially during our build process, we only compile files inside src/, so because the cli.js file was not inside there, the const declarations you added would have thrown exceptions on Node older than v6, and you couldn't change them to var because then our linter rules would have failed.

[existentialism]

Nice work @xtina-starr!

[xtina-starr]

Ah, good to know for the future!

and thanks @existentialism 😃

[alloy]

Great read, this back and forth, and congrats for landing this patch @xtina-starr 👏

[yinkaihuang]

i have get a program when start Project 。npm -start
the follow messge print:
You have mistakenly installed the babel package, which is a no-op in Babel 6.
Babel's CLI commands have been moved from the babel package to the babel-cli package.

npm uninstall babel
npm install --save-dev babel-cli

See http://babeljs.io/docs/usage/cli/ for setup instructions.

so how can fix them