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 the locally installed version of AVA. #277

Merged
merged 1 commit into from
Nov 29, 2015

Conversation

jamestalmage
Copy link
Contributor

This hunts for AVA in the locally installed project, and runs that
CLI instead of the globally installed one.

Fixes #157.

@jamestalmage
Copy link
Contributor Author

The second commit wraps the entire CLI in an IIFE, to avoid a ParseError from xo.

eslint/eslint#1158

@jamestalmage jamestalmage force-pushed the prefer-local-over-global branch from a8b0c20 to d6c3b6e Compare November 28, 2015 00:18
@jamestalmage
Copy link
Contributor Author

If xojs/xo#51 were to be implemented, the second commit would not be necessary.

@jamestalmage
Copy link
Contributor Author

This needs a test suite, which is doable but will involve a number of fixtures. I would like agreement on my general approach before completing.

Right now, it outputs the following:

  • Run locally, or globally without a local version: no warning issued

  • Run globally, with a local version:

    Using local install of AVA (v0.6.1), which differs from global (v0.6.0)
    

    or

    Using local install of AVA (v0.6.0), which matches global (v0.6.0)
    

@sindresorhus
Copy link
Member

Instead of wrapping it in an IIFE, just remove XO for now. We can add it back when we've sorted this out.

@sindresorhus
Copy link
Member

Right now, it outputs the following:

I don't think it matters if they differ. All we're doing with the global CLI if local is available is to require the local one. As long as the user has AVA 0.7.0 containing this pull request installed globally they should be fine.

// Prefer the local installation of AVA.
var resolveFrom = require('resolve-from');
var localCLI;
try {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Been thinking about changing resolve-from to just return null when not found instead of throwing. Thoughts?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think resolve-from should adopt that behavior. I can't think of very many scenarios where you would want resolve-from, and not want to wrap the thrown exception.

Also, it looks like someone was already assuming that behavior here: babel.js#L41. This looks like the more common use-case.

Perhaps retain the old behavior somewhere:

require('resolve-from/throws');

or something like that.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I'll unpublish try-resolve-from. sindresorhus/resolve-from@4cb3cf0

@jamestalmage
Copy link
Contributor Author

Instead of wrapping it in an IIFE, just remove XO for now. We can add it back when we've sorted this out.

This is what led me to the shim behavior in fallback-cli, it allows you to do this:

// cli-shim.js
require('fallback-cli')('ava/cli.js', function (opts) {
  // returns are allowed inside the callback - linters stay happy.
  if (opts.globalCli && opts.localCli) {
    console.warn('Using the local install of AVA instead of global.');
  }
  require(opts.cli); // opts.cli === opts.localCli || opts.globalCli
});

@jamestalmage jamestalmage force-pushed the prefer-local-over-global branch 4 times, most recently from 335708a to db8d79f Compare November 28, 2015 06:21
} catch (e) {}

if (localCLI && localCLI !== __filename) {
console.warn('Using local install of AVA.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need that warning, it gives no helpful information to the test output. Plus, it will not look good.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is something we could have in a future --verbose mode or just add it as a debug()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Honestly, that line was there to appease you: xojs/xo#32 (comment).

I thought you brought up a good point, that silently changing standard behavior is a bad thing.

@sindresorhus, what do you think?

@sindresorhus
Copy link
Member

@jamestalmage Bump resolve-from and it will no longer throw ;)

@sindresorhus
Copy link
Member

Generally looks good to me.

@jamestalmage jamestalmage force-pushed the prefer-local-over-global branch from db8d79f to cb58a1a Compare November 29, 2015 06:35
var localCLI = resolveFrom('.', 'ava/cli');

if (localCLI && localCLI !== __filename) {
if (debug.enabled) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have to check if debug is enabled. That's already done by debug

@sindresorhus
Copy link
Member

One minor comment. Otherwise it's good. Feel free to merge when that's resolved.

This hunts for AVA in the locally installed project, and runs that
CLI instead of the globally installed one.

Fixes avajs#157.
@jamestalmage jamestalmage force-pushed the prefer-local-over-global branch from cb58a1a to 2f7ac92 Compare November 29, 2015 07:14
jamestalmage added a commit that referenced this pull request Nov 29, 2015
Prefer the locally installed version of AVA.
@jamestalmage jamestalmage merged commit f13ba2e into avajs:master Nov 29, 2015
@jamestalmage jamestalmage deleted the prefer-local-over-global branch November 29, 2015 07:41
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.

4 participants