bensu / doo

doo is a library and lein plugin to run cljs.test on different js environments.
Eclipse Public License 1.0
324 stars 63 forks source link

Use karma-cli as part of default config #46

Closed danielcompton closed 8 years ago

danielcompton commented 8 years ago

Karma recommends that people install Karma locally, and install karma-cli globally to help them run Karma without having to call ./node_modules/karma/bin/karma start, and to ease cross platform compatibility with Windows.

This PR updates the documentation to recommend this installation approach, and changes the default Karma path from ./node_modules/... to karma.

One thing I'm not sure about is how to combine non-standard node_modules path (e.g. in subdirectories), with karma-cli. However I think that can be a secondary concern?

bensu commented 8 years ago

Hi @danielcompton, thanks for your work!

There a three things to consider before applying this change and from where I'm standing it wouldn't be desirable to do so:

  1. Can you point me to those recommendations? When I made the current default decision, I was following the advice given here which completely resonates with my experience (working with other developers and sharing config via package.json).
  2. It's not backwards compatible (which implies that reason for change needs to be very compelling).
  3. It is much easier to correctly configure :paths {:karma "karma"} if you get it globally than to know where to look for if you install Karma locally. Also, local is the default installation mode for npm (no -g required) which makes it even easier to get it wrong from doo.

As for a non standard node_modules path, it can be configured in :paths {:karma "./some/other/node_modules/..."}.

danielcompton commented 8 years ago

Hey @bensu

The main reason why I put this PR together is that I'm trying to roll out doo on our CLJS projects, but they need to be tested from Windows, as well as Mac OS X and Linux. When doo tries to run ./node_modules/karma/bin/karma start, it won't run on Windows. They've also stopped shipping a karma.cmd file in the main Karma project so on Windows you have to use karma-cli if you want to run Karma. See https://github.com/karma-runner/karma/issues/941#issuecomment-37329149 and Stack Overflow for more details.

If this PR isn't merged then we can provide a :paths {:karma "karma"} manually on all of our projects to work around this. You might find others run into the same issue too.

  1. On the linked page

    The recommended approach is to install Karma (and all the plugins your project needs) locally in the project's directory.

    and

    you might find it useful to install karma-cli globally.

    I think they should be a bit more explicit here, as Windows devs have to install karma-cli to use Karma.

  2. You're right it's not backwards compatible, but doo is still very young and on an unstable release, so if you wanted to make breaking changes, now would be the time (in 0.1.7-SNAPSHOT)
  3. I didn't quite understand what you were trying to get across here. Karma is still being installed locally, it's just using a global cli tool to find the correct local Karma.

As for a non standard node_modules path, it can be configured in :paths {:karma "./some/other/node_modules/..."}.

What I was thinking out loud about was how to handle this when you have Windows and Unix computers, as providing a path to "./some/other/node_modules/..." won't work on Windows.

In summary:

  1. If your team is doing cross platform development between Windows and Unix, then you're going to have to use the Karma-CLI
  2. The Karma-CLI is already gently recommended by the Karma team
  3. With my instructions (perhaps with some edits), everyone using doo will install things in the same way which should cut down on support issues.
  4. If you have two different installation paths, one for Unix, and one for Unix and Windows, then this may cause confusion and support issues.

I totally understand if you don't want to make a breaking change, and I definitely appreciate how nice it is to have all of your dependencies locally specified, without needing to install anything global. If you'd prefer, I can put together a different PR which advises people doing cross platform development to use karma-cli.

I also need to put together another PR after this to help doo call Karma on Windows.

bensu commented 8 years ago

Great response, very clear where the pain points are. Let's address them:

  1. (crossplatform) Addressed with 4
  2. (backwards compatibility) I agree. There are probably breaking changes somewhere due to all the karma related refactoring, but no API/config changes. If possible I'd like to keep it this way, but as you said, it's not crucial.
  3. (me being unclear) I meant, since people install by default locally (npm install karma no -g needed) and it's easier to gt this right {:path {:karma "karma"}} than this {:path {:karma "./node_modules/..."}} I think it makes sense to be aligned with npms default installation mode.
  4. (crossplatform) The underlying issue is that Karma's Window support is asymmetric and as of 0.12 you need to use a global path in Windows. If this changes (and local Karma is supported on Windows), we could definitely rearrange the default path to be Windows compatible, i.e., (.getPath (io/file ".node_modules" "karma" "bin" "karma')). This puts in evidence that the current implementation of :paths is not suitable for a crossplatform team and that is now an issue in itself, #47.

We should follow your suggestion: document the upstream issue and advice Windows and cross platform users to add :path {:karma "karma"} and install karma-cli. If you can add those docs great! Otherwise I'll get to it over the weekend and ask you for feedback since I'm not a Windows user myself.

Hopefully if somebody creates a karma.bat, we will have #47 solved by then and allow your team to be cross platform and use a local config!

danielcompton commented 8 years ago

It is definitely recommend to use karma-cli.

:point_up: October 23, 2015 12:33 PM

OTOH

no specific recommendation, depends on your preference really https://github.com/karma-runner/karma/pull/1658#issuecomment-150376980

danielcompton commented 8 years ago

Following up on this, I got a really confusing error message on Windows when I ran doo without setting :paths {:karma "karma"}. Even though I wrote the PR for Windows support, it took me a while to realise what was happening :)

'.' is not recognised as an internal or external command,
operable program or batch file.

I'd argue it would be better to encourage use of karma-cli as the default installation method to ease confusion by others, and to get a consistent runner command for all environments. The test I was running above worked fine on unix, but failed on Windows. Docs would be another option, but is a little less preferable in my opinion.