feat(config): Add preprocessorOrder option to config for when prepr…

[sjelin]

…ocessors

must be run in a specific order.

Some karma users at Google discovered karma-coverage needs to be run before
karma-googmodule-preprocessor, hence we need this feature.

This required an update to karma-browserify, which can be found here: nikku/karma-browserify#168.

Since this version is not yet released, this will break travis.

[dignifiedquire]

Please remove this, version bumps are generated automatically on release

[sjelin]

Done. The problem now is that I'll have nothing to reference in the browserify change

[nikku]

I'd like to put a different solution for this on the table: Allow config.preprocessors to be an array instead of an object. This way the processing order for individual preprocessors is clear from the start and no additional configuration property is needed.

Example JSON:

{
  "preprocessors": [
    [ ".js", "googmodule" ],
    [ ".js", "coverage" ]
  ]
}

@sjelin Could you provide us with an example project + karma configuration that fails with the current version of karma?

[sjelin]

@nikku we were having problems with a config file like:

"preprocessors":  {
  "**/*.js": [ "googmodule" ],
  "**/client/**/*.js": [ "coverage" ]
},

Which caused googmodule to run before coverage, which in caused problems in the coverage information (not sure exactly why).

The problem with the preprocessors array option is that you'd run into problems combining multiple config files if one assumed preprocessors was an array and the other assumed preprocessors was an object.

[dignifiedquire]

@nikku have you tried doing sth like this:

"preprocessors":  {
  "!(client)/**/*.js": [ "googmodule" ],
  "client/**/*.js": ["googmodule", "coverage" ]
},

You would need to explicitly define the top level path, but you wouldn't need to add any code to karma to get this working.

[sjelin]

Yes, that would probably work in this instance. But users may have a more complicated group of files they need coverage data for. For instance:

"preprocessors":  {
  "a/**/*.js": ["googmodule", "coverage" ],
  "**/b/*.js": ["googmodule", "coverage" ],
  "c*d/*.js": ["googmodule", "coverage" ],
  "<else *.js>": [ "googmodule" ]
},

In this case I have no idea what pattern goes in the <else *.js> case. I'm also not sure what would happen to a file like a/b/x.js, which ends up getting ["googmodule", "coverage", "googmodule", "coverage"] run on it.

Plus, while it's good that it requires no karma code change, it feels like a hack. I want googmodule to be run everywhere, and it should be simple to write that in the config.

[dignifiedquire]

Can we avoid this? I'd rather not risk breaking other preprocessors

[sjelin]

I guess so. We could use a custom property of config like !!preprocessorOrder which would never be needed to match a real file. I'm also open to any other ideas.

[nikku]

@sjelin Ok, how does the following (switched order of preprocessor definitions) behave then?

"preprocessors":  {
  "**/client/**/*.js": [ "coverage" ],
  "**/*.js": [ "googmodule" ]
},

[sjelin]

I mean, that works (sometimes at least), but it's not supposed to work. It relies on Object.keys to return the keys in the same order they're written down in, but the spec explicitly says Object.keys returns the keys in an arbitrary order.

[dignifiedquire]

@sjelin what happens if you do:

"preprocessors":  {
  "**/*.js": [ "googmodule" ],
  "**/client/**/*.js": ["googmodule", "coverage" ]
},

[sjelin]

I think you mean:

"preprocessors":  {
  "**/*.js": [ "googmodule" ],
  "**/client/**/*.js": ["coverage", "googmodule"]
},

since coverage is supposed to run first.

Currently there is no de-duplication in preprocessor.js, so it would run either ["googmodule", "coverage", "googmodule"] or ["coverage", "googmodule", "googmodule"], depending on the order of the Object.keys array. I could write some kind of intelligent de-duplication/ordering into preprocessor.js. The advantage is that it wouldn't change the function's signature or introduce new properties into the config file. The disadvantage is that it would just be a secret version of preprocessorOrder

[dignifiedquire]

Yes that's what I meant.

I think it would be a very good solution to introduce deduping here and use that order as it would be much more explicit and would solve the more complex use case where you want different orders of the same preprocessors in different files.

As long as we properly document this it wouldn't be a hidden feature.

[sjelin]

Ok, I'm good with that.

[nikku]

+1 if you get that properly explained to people, too.

[zzo]

thanks sammy fix travis :)

[sjelin]

Ok, I wrote it the other way. I know what you're going to say: "Why is it so complicated?" It turns out merging arrays, trying to keep their orders in tact as much as possible, isn't easy. I'm more than open to similar solutions.

[dignifiedquire]

😆 who would have guessed we find some big O notation in here one day

[dignifiedquire]

Style: Please use // for comments to be consistent with the rest of the code

[dignifiedquire]

@sjelin nice work, didn't expect this to get that complicated though :D

I left some style nitpicks but other than that I'm pretty happy with this solution

[dignifiedquire]

Also the commit message needs to mention that this is a breaking change, as it might change the order of preprocessors for some expecting the old behaviour.

[sjelin]

All comments addressed! Though tbh once I split the list merging into its own repo (https://github.com/sjelin/combine-lists), most of the comments were no longer relevant.

[dignifiedquire]

Thanks, nearly there, looks like linting is unhappy now. (grunt lint locally)

[sjelin]

Fixed the lint problems. Looks like some of the e2e tests are failing. Are they flaky though? It seems pretty random

[dignifiedquire]

@sjelin restarted them, yes they are super flaky atm sometimes travis gets so slow they time out :(

[dignifiedquire]

Thanks