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

Prefer locally installed version, fall back to global. #32

Closed

Conversation

jamestalmage
Copy link
Contributor

Use whatever version of xo is installed locally in node_modules, before using the globally installed version.

Fairly naive implementation, but it works and is backward compatible as far back as I checked (0.7.x).

I disable update-notifier if a local version is found. That could maybe be improved upon by still notifying if local is out of date. update-notifier seems to always recommend installation with the -g flag though.

@shinnn
Copy link

shinnn commented Sep 26, 2015

Do you really need this feature despite npm scripts automatically run locally installed binaries? https://docs.npmjs.com/misc/scripts#path

@jamestalmage
Copy link
Contributor Author

Yes. npm run lint is an excruciating 10 characters more verbose than xo. 😉

@jamestalmage
Copy link
Contributor Author

I am working on fallback-cli, which will do basically the same thing in a generic way. It's almost ready (just need to ad a couple more tests and some docs).

@jamestalmage
Copy link
Contributor Author

Switched PR to use fallback-cli.

@joakimbeng
Copy link
Contributor

Yes. npm run lint is an excruciating 10 characters more verbose than xo. 😉

I simple solution to this is prepending ./node_modules/.bin/ to your PATH. Then xo will run the local binary if it exists and the global otherwise, no other module needed.

@jamestalmage
Copy link
Contributor Author

@joakimbeng Huh, I guess I had always just assumed PATH required absolute paths. Thanks for that!

Unfortunately, it doesn't work for me. I use nvm to manage multiple versions of Node on my machine, and it prepends to the path variable whenever you change versions, so globals would end up taking precedence again with every switch.

@sindresorhus
Copy link
Member

Not a big fan of this tbh. This way every tool has to change instead of you once. This will also break when global and local CLI differ, which will definitely happen at some point. It also makes it ambigious when the local/global one is used and makes it a lot harder to debug issues coming up.

I've just aliased it in my dotfiles:

  • alias lint=npm run lint
  • alias nr=npm run
  • alias nt=npm test

I would rather see this solved for all tools through npm-exec. Then alias e=npm-exec$ e xo.

For reference, relevant ESLint discussion → eslint/eslint#3993.

@jamestalmage
Copy link
Contributor Author

This will also break when global and local CLI differ

That entirely depends on what you mean by "break". If you are saying that version X won't handle the same options as version Y, then yeah, obviously. The fallback mechanism won't fail, but the options you can use with each version of the CLI will obviously change over time.

This solution remains backwards/forwards compatible as long as you never change the relative path to ./cli.js. The shim does not process args for you, so it won't break those. In the eslint example it works all the way back to 0.0.4. You have to use the cli options appropriate to a given version when you call it.

It also makes it ambigious when the local/global one is used

To the contrary, it disambiguates things. If local is present, it is always what is used. Currently, you have a confusing scenario where two identical commands produce different results depending on if they are executed from an npm script or the command line.

It makes it harder to debug issues

I do not see how it is any harder than the current situation. Currently if a user submits an issue reporting that "xo produces ... ", your first step needs to be disambiguating if they are invoking xo from the command line or an npm script, and following up by asking which local/global version they have installed. With this change you can just ask them to run xo --version from their project directory, and you know which version is linting their code.

npm-exec ... alias

That is all fine and good when it actually happens. Still, code within a given project is intended to be linted/styled against a specific version of a specific linter.

This way every tool has to change instead of you once

Not sure what to make of that.

@sindresorhus
Copy link
Member

@vdemedes @floatdrop @SamVerschueren @kevva Could really use your thoughts on this. If I decide to use it here, I'm gonna use it in AVA too.

@SamVerschueren
Copy link
Contributor

Isn't it possible to do this a little bit cleaner? It adds quite some overhead to your modules imo. But not sure if can be done otherwise.

Why the shim? Can't the fallback-cli be required in the cli.js (just like update-notifier)? This way you can keep the bin property in package.json. Instead of passing xo/cli as path to fallback-cli, just read the contents of package.json. If the cli name changes, you don't have to do anything else then change the bin property.

Not sure if possible, hard to figure out on mobile :).

@jamestalmage
Copy link
Contributor Author

@SamVerschueren The shim is necessary. Here's the default process:

  1. global cli-shim.js gets loaded and calls fallback-cli
  2. fallback-cli.js tries to locate the local xo/cli (by resolving from the cwd).
  3. If it finds the local one, it requires it - thereby kicking off the cli in the local context.
  4. Otherwise it requires the global cli (in the same directory as the shim) and kicks off the cli in the global context.

The important thing to note is that when you call the global cli-shim and it finds a local instance, it requires the local cli (not cli-shim again). This bypasses a second execution of fallback-cli and avoids a sort of loop.

As for parsing package.json, that falls apart if you specify multiple commands (i.e. mocha and _mocha). Perhaps a future update to fallback-cli should include a no-arg option that does do it.

Not a concern here, but for ava we may want/need to take advantage of specifying a custom runner (scroll up a bit to from the link to see the default example). My thinking there is that if there is no local installation of ava then we should not be looking in the local node_modules folder for babel-core/runtime, etc. A custom runner could pass along an executingGlobally flag that disabled some of our lookup paths.

The only real downside is that your global cli changes behavior depending on directory. In a sense, that is unexpected and potentially confusing. "Why is --flag not working? OH! the local install is an old version, and that isn't supported". But I use a number of tools that employ the "default to local" strategy, and I have never been (badly) burned by it.

I have been burned repeatedly by the other situation. ava/xo/eslint all set up with a local install and npm scripts to automate the build, but when I type the global ava command it doesn't work. I then assume something is broken locally and go hunting around trying to fix it, and it doesn't dawn on me for a while that I've got some global vs local dependency conflict. (I'm in the local directory, I see the local install of ava right there, and my brain just gets stuck on the wrong conclusion).

With xo, I have seen this confusing behavior where a repo that has an outdated dependency on xo (and has been passing in CI) suddenly starts reporting a bunch of lint errors because I upgraded global and was just calling xo. That was fairly easy to figure out. However this had me scratching my head for a long time before it dawned on me what was happening.

@SamVerschueren
Copy link
Contributor

Not have much time at the moment, will have a closer look soon. Thanks for explaining!

@vadimdemedes
Copy link

I feel strongly against this, because it introduces a lot of confusion. There should be a clear differentiation of what's local and what's global, as it is now.

I wouldn't mess with this just to save a few characters. As @sindresorhus and @shinnn pointed out, you can use npm scripts or bash aliases for that.

@jamestalmage
Copy link
Contributor Author

it introduces a lot of confusion.

People keep saying that, but I still do not buy it.

If I run the command xo in a project that is already configured to lint with xo; What scenario is there where I actually want to use some different version than what is configured?

@vadimdemedes
Copy link

@jamestalmage Here's absolutely common scenario (at least for me), which I always use when developing CLI tools. Say I implemented some new stuff in xo CLI and I want to test it. I run npm link so that my branch is now globally linked and I go to my project's directory to test it. But if xo always uses locally installed version, I will not be able to test my changes.

@jamestalmage
Copy link
Contributor Author

Ok, so we change package.json to this

"bin": {
  "xo": "cli-shim.js",
  "_xo": "cli.js"
}

And then you can just _xo to grab the global one.

Alternatively we could add a --force-global hook (totally easy to do).

Or... What if I can detect that it is globally linked and automagically ignore the local version? Not sure if it's possible, but I think it probably is. Might be difficult. But if that's what it takes I'll try it.

@vadimdemedes
Copy link

And all these hacks are just to avoid typing a few more characters?

@jamestalmage
Copy link
Contributor Author

No. To avoid confusing scenarios for users.

@vadimdemedes
Copy link

I think it will only avoid confusing scenarios for beginners, but introduce confusion among experienced developers.

@jamestalmage
Copy link
Contributor Author

Can you describe a scenario where it might confuse a developer (outside someone who is hacking on xo itself).

@vadimdemedes
Copy link

Because this PR breaks standard behavior. If I execute $ xo, I know I am executing globally installed xo. If I execute $ ./node_modules/.bin/xo, I know I am executing locally installed xo.

This just introduces black magic under the hood, which will confuse experienced Node.js developers and they'll wonder "why the hell does this command uses a locally installed version", "I am not using npm scripts, why is it using a local version", "I am not using relative path, why is it using a local version".

To sum up, there is only one reason for it and many reasons against.

People participating in this discussion pointed out good solutions on how to save characters and use local xo versions. They are standard, they are straightforward and they don't require tweaking the xo codebase.

@jamestalmage
Copy link
Contributor Author

@vdemedes I've read and given careful consideration to every response. I'm not here to stir up trouble, but to have a healthy debate. I hope you are not taking it that way.

Speaking frankly, IMO you have only provided one good example of a scenario where someone would actually NOT want the behavior proposed (yourself as you hack on xo CLI).

This whole section of your response:

"why the hell does this command uses a locally installed version", "I am not using npm scripts, why is it using a local version", "I am not using relative path, why is it using a local version".

... is unconvincing unless there are scenarios where people actually want that. If I have a codebase that is currently linted against [email protected], why would I ever want to run my globally installed [email protected] against it? If I decide it is time to upgrade xo, then I am going to do so locally.

@vadimdemedes
Copy link

To sum up and make this as clear as possible one more time, my concerns are related only to not altering standard behavior.

@sindresorhus
Copy link
Member

Alright, we eventually came to the conclusion in AVA that this can actually be useful.

@jamestalmage Can you update to use the same method as used in AVA?

@jamestalmage
Copy link
Contributor Author

closing in favor of #57

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

Successfully merging this pull request may close these issues.

6 participants