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

Version warning when only local version is behind. #1336

Closed
jamestalmage opened this issue Apr 3, 2017 · 12 comments
Closed

Version warning when only local version is behind. #1336

jamestalmage opened this issue Apr 3, 2017 · 12 comments
Labels
bug current functionality does not work as desired good for beginner help wanted

Comments

@jamestalmage
Copy link
Contributor

If the global version is up to date, but the local version is behind, you still get an error message telling you to update, but it recommends using the -g flag.

@novemberborn novemberborn added bug current functionality does not work as desired good for beginner help wanted labels Apr 4, 2017
@thisisrai
Copy link

@jamestalmage super interested in helping out on this issue, but I'm not sure on where to start.

@novemberborn
Copy link
Member

AVA starts with cli.js. It tries to detect a local version, and loads ava/cli from that version instead. Otherwise it runs the CLI from ./lib/cli.js.

That file runs update-notifier (see code). We should provide {isGlobal: false} as an option to notify() to fix the version warning.

I suggest adding an environment variable in cli.js:

if (!process.env.AVA_LOCAL_CLI) {
  process.env.AVA_LOCAL_CLI = String(localCLI)
}

When running the local CLI that should be "true", but when running the global CLI without a local AVA installation it should be "false" (since it cannot find the local CLI). Then in lib/cli.js you can compare the environment variable against "true".

At least I think so, this logic is mind-bending without running some tests 😉

@undefinedTea
Copy link

is this being worked on, or available still?

@novemberborn
Copy link
Member

@thisisrai have you had a chance to make progress on this?

@servicelevel please feel free to make a start. It's possible of course that @thisisrai is nearly done. Regardless I'll add the assigned label.

@undefinedTea
Copy link

i'll give @thisisrai a few hours to respond here, and then i'll get it going if its not already in progress/near completion.

@thisisrai
Copy link

@novemberborn I haven't had a chance to look it at yet. Been super busy lately.

@servicelevel Feel free to go for it.

@novemberborn
Copy link
Member

@thisisrai no worries. Thanks for responding! 👍

@undefinedTea
Copy link

undefinedTea commented May 9, 2017

alright. on it...
the approach @novemberborn suggested above seems reasonable, so i'll go with that.

i suppose notify() when given the {isGlobal: false} option will still provide the desired prompt, only without the --global flag, so this should be fine.

i am wondering if there should be an indication as to whether the global or local version of the cli was used, but that may be another discussion all together.

@timdeschryver
Copy link
Contributor

timdeschryver commented Jun 5, 2017

@servicelevel Are you working on this issue?
If so, do you need some help or can I take this one?

@undefinedTea
Copy link

damn - same faith as @thisisrai :/ just 0 time.
i did 'complete' the actual code, but never managed to get around to test it properly or write tests.

so please do take it @tdeschryver - and if you want, i can commit my code later (missing tests as mentioned) but its a rather well outlined approach by @novemberborn above, so not sure that would help much.

@timdeschryver
Copy link
Contributor

timdeschryver commented Jun 6, 2017

@novemberborn I'm having a small problem with the testing. At test/cli.js you can see I'm deleting the process.env.AVA_LOCAL_CLI property because otherwise its being used in the next tests (if its been set, we just leave it as it is - cli.js) which will then fail. Is there a better way of doing this, this seems a bit dirty to me? I've tried tap.beforeEach and tap.afterEach but without success...

@novemberborn
Copy link
Member

@tdeschryver

Is there a better way of doing this, this seems a bit dirty to me?

You could change lib/cli.js so it does require('process') and then stub it… but I'd be fine with what you have currently as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug current functionality does not work as desired good for beginner help wanted
Projects
None yet
Development

No branches or pull requests

5 participants