Extract watcher into an injected dependency

[cowboyd]

I'm writing a plugin that integrates Karma with Broccoli so that it can lean fully on broccoli for all pre-processing, building, etc... The reason being that if you have a Brocfile, then you already have your build toolchain defined in one place, so you might as well utilize it from your test runner. That way, instead of maintaining a parallel build systems: one for your test runner and one for development/production, you can just npm install karma-broccoli and it will know how and when to build, and also which files the build generates. I have the single run use-case working where you invoke karma run. It waits until a build runs (if there is a build), and then executes the test suite.

For the config.autoWatch use-case, I'd like to replace the normal karma watcher with the Broccoli watcher for the same reason I want Broccoli to be responsible for the build, it already knows what to to watch for and consequently, what to build. It would be nice if I could just swap out the broccoli watcher for the bundled karma watcher, but at the moment it is hard coded to the watch function in watcher.js https://github.com/karma-runner/karma/blob/master/lib/server.js#L54

While the dependencies to the watch() function are injected, the watcher itself is not, so I'd like to see if you would be open to extracting the watcher into a first-class injected dependency. If so, I'd be happy to take a stab at implementing it.

[dignifiedquire]

Hey, thanks for the suggestion, I think it's a very good one. In my mind the optimal solution would be

• extract the current watcher into it's own (npm) module karma-file-watcher
• define and document an interface for the api the watcher needs to expose
• set the default config to use the karma-file-watcher but allow for any other module to take its place.

Does that sound good to you? If yes I'd be very happy if you want to work on this as I probably won't find time any time soon to do this.

[cowboyd]

Yeah, that sounds like a good plan, and I can get started right away.

How about I document the interface to watcher in the README for karma-file-watcher. Also, should I extract the file-watching tests to that repo, or is the understanding that

Finally, It seems to me something like karma-file-watcher should be owned by the karma project and not me personally. I assume that at some point I can hand off the repo before merging in the follow-on PR?

[cowboyd]

After poking around, it looks like the watcher does its communication primarily through the file list. In other words, it calls fileList.{add,change,delete}File. The FileList emits an file_list_modified event which is consumed in 3 places:

lib/reporter.js

emitter.on('file_list_modified', function(filesPromise) {
  filesPromise.then(function(files) {
    lastServedFiles = files.served;
  });
});

lib/web-server.js

  emitter.on('file_list_modified', function(files) {
    filesPromise.set(files);
  });

lib/server.js

if (config.autoWatch) {
  globalEmitter.on('file_list_modified', function() {
    log.debug('List of files has changed, trying to execute');
    executor.schedule();
  });
}

The last being the critical one that actually schedules a test run. So clearly, there is some coupling of the watcher to the fileList in order for the web-server and reporter components to work correctly. The problem is that the broccoli watcher does not (that I'm aware of) let you know when a file is added or removed. It just tells you that the build is complete once the final directory is ready with your distribution. Is it ok then to have the watcher emit a file_list_modified event directly? or should there be some contractual obligation with the fileList? If so, then I'd need some sort of fileList.replaceFiles() so that I can do a full refresh every time.

Perhaps I don't fully understand the role of the fileList though.

[dignifiedquire]

Re repo, yes I'll set up one that we can use. Re tests, the unit tests for the watcher should be moved into the new package I would say, and some basic tests checking that the default watcher works should be added to the main repo.

Take a look at the rewrite of file-list I did in the canary branch, that hopefully makes it easier to understand how things are working. I'll take a look later, on the exact details on where the cut should happen.

[NicBright]

I (think) I've thoroughly studied all the relevant files:

1. new file-list
2. runner.js
3. middleware/runner.js
4. watcher.js
5. server.js
6. plus the integration with grunt-karma

And I'm not entirely sure whether the watcher should be extracted at all, because:

1. the file-list is the main data structure that triggers a test run (via "file_list_modified" event)
2. the karma-file-watcher (watcher.js) merely interacts with the file-list
3. there is already an externally exposed API that enables a 3rd party watcher to do the same thing as 2., i.e. runner.js + middleware/runner.js (it was introduced to support the WebStorm integration): while runner.js can be conveniently required as is the case for grunt-karma (e.g.), middleware/runner.js exposes the API externally, as any (external) component can send an appropriate HTTP request to the karma server.
4. this integration already is independent of the server process, i.e. the server can be run in a background process (let's call it _B_) while the "main" file watcher (e.g. WebStorm, or grunt-karma) can require runner.js and call runner.run(...) in the main process (let's call it _A_). Simply extracting the current karma watcher as a "karma-file-watcher" would not allow this.

Put differently: The karma server needs to be able to run as a background process (because otherwise it would block consecutive tasks as is the case for grunt-karma if one starts the server without "background: true"). Assuming that the 3rd-party watchers (e.g. karma-broccoli) run in their own process, in order to cleanly extract the watcher, it needs to be able to communicate with the server process somehow (directly binding to fileList would no longer be possible). And this is functionality that's already implemented by middleware/runner.js.

Conclusion

In my understanding, watcher.js should be considered an integral core component of karma (**) that enables karma to be used standalone. As soon as karma is used as part of a build tool / task runner, it is no longer necessary to use watcher.js at all. And this comes pretty easy and naturally, as one just needs to not use the "autoWatch" option.

Instead, I think it makes more sense to extend middleware/runner.js (+ runner.js) to expose more of the file-list's API. For example, there is a method called "reload" available, that is not used anywhere: https://github.com/karma-runner/karma/blob/v0.13.3/lib/file-list.js#L262 It'd be very nice if this method would be exposed as it perfectly supports my use-case from https://github.com/karma-runner/karma/issues/1507

(**) or maybe a standalone plugin, see https://github.com/karma-runner/karma/issues/1050

[wied03]

Adding to what @NicBright said, what if you allowed the fileList to be externalized? Or at least add a allow plugins to specify a "preprocessor" for the FileList such that you can alter the list (asynchronously, including adding additional files and preprocessing them) before FileList emits the file_list_modified event?