karma-runner / karma

Spectacular Test Runner for JavaScript
http://karma-runner.github.io
MIT License
11.96k stars 1.71k forks source link

Export binary #1078

Closed necolas closed 9 years ago

necolas commented 10 years ago

The binary export was removed in this commit: c7d46270aca83ecfe78f69fa923bc574c0b5bfdc

But I think this has broken npm-scripts aliasing. Normally you can run the local install of a package from npm-scripts in package.json (because npm looks up the binary in ./node_modules/.bin/:

{
  "scripts": {
    "test": "karma start"
  }
}

But this doesn't work with Karma anymore.

maksimr commented 10 years ago

@necolas you can install karma-cli local, to the project, and this should work how before

necolas commented 10 years ago

That works, but why do we have to do that? Not exporting the binary doesn't make sense to me. Just seems to introduce needless indirection.

aymericbeaumet commented 10 years ago

I believe the original reason for this removal was to force the developers to install the karma-cli package in order to call Karma from the command line, which is fine by me because it gets the proper karma binary wherever the karma command is called.

So I think this level of indirection is justified and IMHO it's great that it's been exported in a separated package so that it leaves karma to only worry about its job.

There is indeed the drawback you mentioned, but it seems ok by me to add the karma-cli dependency in order to call Karma (as suggested by @maksimr).

WDYT @vojtajina?

ghost commented 10 years ago

Agreed with @necolas, this is a bit confusing. I've proposed a warning message is introduced in the related issue referenced by @maksimr.

necolas commented 10 years ago

Yeah, I haven't heard a good reason for not exporting the binary. Breaks people's expectations without any benefits.

maksimr commented 10 years ago

@necolas separation of concerns. Concerns of karma-cli just init, start and run karma from console. karma provides public API for this purposes.

For this reason karma-cli may install global, and simple run several karma instance. May implement tricky init method. All this code does not direct relate to 'core'(karma-cli a tool for using karma, You can implement web interface for starting karma and this does not mean that this code should located in karma) karma. So all of this code can live and releasing separately from 'core' karma. I hope my message is clear. IMHO

Thanks!

necolas commented 10 years ago

The binary is in the repo: https://github.com/karma-runner/karma/blob/master/bin/karma. It should be available for local execution without installing something else.

nzakas commented 9 years ago

Agree with @necolas here. This is pretty confusing and frustrating. If you are shipping the binary in the karma package, why not include an easy way to use it?

peter-mouland commented 9 years ago

Hi, just came across this as i was going to do the same PR. In the docs it says you can use the full path.

https://karma-runner.github.io/0.12/intro/installation.html

Run Karma: $ ./node_modules/karma/bin/karma start

I dont get why i would want the cli when I would like to simply put karma start xx.js into my package.json. Since the bin is in karma, and it works, could we add the bin into karma's package.json?

I think the route of asking people to also install the cli is making the process a bit convoluted. The cli doesn't currently have tests and is only used for karma, so it isn't really a separate project.

1351 #1322 #1275 ... there are few issue open and closed about this!

just my 2cents.

donaldpipowitch commented 9 years ago

Sorry, I don't get it? Just made a pull request myself. That is quite annoying to realize that I wasted my time with this even if it was just some minutes. Installing a different module (karma-cli) is just unexpected and in my experience not idiomatic for a node module.

nzakas commented 9 years ago

@maksimr I'm trying to understand your objection, but I'm having a hard time. From our perspective, your stance is confusing because we are just asking for an easier way to run the binary that is already shipped in this package. As #1361 shows, it's basically a one-line change that would make our lives a lot easier. We aren't asking for any change in functionality, nor are we asking for additional files to be included. This is the logical equivalent of a symlink.

This package has 8x the downloads of karma-cli, which means the majority of users have no need for karma-cli. That, plus the number of issues and PRs related to this issue, seem to indicate a pain point for users. Is there anything we can do to change your mind?

maksimr commented 9 years ago

Guys thanks for your feedback.

@donaldpipowitch your PR once again says that we have a problem we need to solve.

@nzakas if we simple add link to binary in karma, it can lead to confusing behavior when we install karma globally. For example: you have karma version 0.12 in the project but globally you have karma version 0.11 and if you simple type karma in terminal then you run karma@0.11 instead karma@0.12. Yup, you can simple update you global karma to latest version, but what if you have more than one project locally which use different version of karma? In this case you can not have two instance of global version of karma. Yes, you can run test like npm test but now we depend from npm how cli launcher, in old version of npm you can not pass parameters(For example how you run karma run --grep="..." using npm).
(IMHO)

For me the main problem of this changes is a habit, but we really have many requests add karma to binary. I think we should discuss this issue and fix it either add link on binary in karma or update documentation make it more clear.

Thanks

/cc @karma-runner/contributors @zzo @vojtajina

nzakas commented 9 years ago

I understand you are worried about karma possibly meaning two different things depending on where it is used. Would a solution be to expose the binary in this package with a different name? Maybe karma-runner instead of just karma?

peter-mouland commented 9 years ago

my opinion seems to ebb and flow the more i learn, but currently i ask devs of my projects to always use npm commands, ie. npm start, npm test, npm run build etc. This is because that way i make sure i never rely on global installs and i know exactly what version is being run. This has the bonus of the casual dev not caring what technology those steps are using in the background and projects using different technologies still start in the same way.

Would it be acceptable to recommend people install karma locally, then add the karma test .. to the npm scripts object?

nzakas commented 9 years ago

@peter-mouland that's what people are doing already. What we are asking for is a shortcut to the executable. How people choose to wire up their build system to use it is a different story.

peter-mouland commented 9 years ago

thats good - i mentioned it because one reason given for having the karma-cli was due to global vs local installs. If globals are discouraged then this reason is no longer a concern and the package.json bin string can be updated as suggested.

nzakas commented 9 years ago

Yeah, there's definitely a trend towards not doing global installs for CLIs because it makes projects no longer self-contained.

tkrotoff commented 9 years ago

@maksimr

For example: you have karma version 0.12 in the project but globally you have karma version 0.11 and if you simple type karma in terminal then you run karma@0.11 instead karma@0.12.

Very good example of why it is bad to install global npm packages. IMHO this issue is out of scope: this is the way UNIX works (security reasons). Example:

cd ~
echo 'echo "Hello, World!"' > bin/ls2
chmod a+x bin/ls2
export PATH=$PATH:bin
ls2
# => Hello, World!
mv bin/ls2 bin/ls
ls
# => regular /bin/ls output, not ~/bin/ls output

Please, just add back karma to node_modules/.bin, kill karma-cli and stop bypassing the way the OS works (and do not advice for global packages otherwise first year students will have to learn how PATH work and the which command).

If you really believe this is a critical "issue", then propose the npm team a fix for all npm packages.

maksimr commented 9 years ago

@tkrotoff Thanks

I like idea export binary with different name like karma-bin or karma-runner How suggested @nzakas

Then it should be clear if you use karma-bin then you use a specific version of karma.

donaldpipowitch commented 9 years ago

I'd like to see a solution here soon or else more pull requests will drop in. Here is one from 14 days ago: https://github.com/karma-runner/karma/pull/1384

zzo commented 9 years ago

My initial thought reading through all of this is to dump the global karma-cli completely and put the link in .bin/karma and be done. A project has installed a specific version of karma they should use the 'karma' executable that comes with it. I don't get why anything should be installed globally. That seems to be the root of the problem. Especially since the 'karma' executable is already being shipped with the 'karma' package.

necolas commented 9 years ago

Any chance the sensible, common thing can be done? Or shall I just close this year-long thread?

zzo commented 9 years ago

ha depends what you definition of 'sensible' is of course. I'm having problems releasing a new version of karma - once that gets sorted out I will apply the patch linking 'karma' in node_modules/.bin and release it

nzakas commented 9 years ago

@necolas I'd suggest leaving it open. It seems @maksimr is open to this, we just need to gently nudge along. :)

necolas commented 9 years ago

Great, thanks

ghost commented 9 years ago

Resolved with #1384. Please close this issue.

necolas commented 9 years ago

Thanks