Add back housekeeping updates

[taion]

Per #2492 (comment), separate from the question of ES5 v ES6, this had some actual config updates like not adding coverage instrumentation to standard testing runs that I think we want for now.

[mjackson]

Please don't merge yet. I'll put my comments inline.

[mjackson]

I'm intentionally only linting the modules and examples directories here. We don't need to lint everything.

[taion]

Actually, looking at this a bit more, what's the downside of this setup?

The logic for the Karma configuration isn't actually split between the two files - as you can see, karma.conf.js is just a shell out to the actual ES6 file: https://github.com/rackt/react-router/blob/v1.0.0/karma.conf.js

Unlike e.g. the webpack config, the Karma config actually has non-trivial logic - there are separate branches for handling the coverage v. non-coverage case, as well as the two cases when using BrowserStack.

We clearly want some code style guidelines here (and checks to avoid more obvious things like undefined or unused variables). Why not just use the same one as the rest of the repo? The stub Karma config isn't going to confuse anybody who's seen Babel before.

[mjackson]

The downside is that there are two files for configuring karma, which we don't need. Please narrow down this PR to only include the coverage fixes.

[taion]

There are not two files for configuring Karma. The entirety of karma.conf.js is the re-export:

require('babel-core/register')
module.exports = require('./karma.conf.babel.js')

If there were configuration in both places, then I would be just as concerned as you.

Part of the reason I made this update across both repos was actually in preparation of splitting out the common parts - we currently have e.g. the BrowserStack configuration copy/pasted across the repos, which seems extremely suboptimal for maintenance.

Likewise for the webpack configs, which have drifted apart from each other in things like how they apply Babel (and in the plugins used).

To me, one of the major benefits of code as config in the same language used is absolutely the consistency benefits offered.

Especially for the Karma config which has (and needs to have) non-trivial logic, it makes a lot of sense to me to use the same code quality tools we already have to at least do a first-level check of sanity.

For something like a webpack config standing in isolation, this might not be worth it, but do you agree that the cost of a stub two-line re-export is not the same as that of actually splitting the code? And additionally that we probably should factor out common config elements like the BrowserStack setup anyway?

[taion]

I'll re-do these PRs for now, but I do think we should make those changes to the config files - I put a fair amount of effort into looking through the configs line-by-line and e.g. matching things back up to new karma init templates, and I think for maintainability we ought to factor large chunks of those configs into a separate config package anyway.

[mjackson]

do you agree that the cost of a stub two-line re-export is not the same as that of actually splitting the code? And additionally that we probably should factor out common config elements like the BrowserStack setup anyway?

No, I don't agree. I don't think that any of this needs optimizing, and I actually think it can be harmful to refactor this kind of thing out into common packages elsewhere. If we did that, then I'd have to open yet another file, probably in a separate repo entirely, just to know how the tests are running.

[taion]

That's a good point. I think my 2 main concerns here were

• Drift in the configs that should be the same slowly getting out of sync, which I think has already happened a bit between React Router and history
• Managing multiple different separate configs for webpack, where you wanted linked configs for generating docs v. generating the actual code (this is useful if you embed examples on the page, since it ensures that you're using compatible setups everywhere)

The former is something I'm okay with just manually managing. The latter isn't an issue yet, although if e.g. we do make a full-fledged docs site, it's something worth thinking about - even within a single repo, we have duplication between e.g. the webpack config used for Karma and the webpack config used for the UMD builds.

Anyway I'll re-PR this as ES5, and we can follow up on the config management issue separately.

[taion]

Updated.

[mjackson]

You're still doing way too much in this PR. Just add back the coverage fixes. Don't refactor karma.conf.

[taion]

Does this decrease legibility? I already had things set up this way. I think it's a bit of an improvement to separate out the BrowserStack stuff - makes it a lot easier to tell what the "actual" Karma configuration is.

[taion]

I understand this PR is doing a lot - the original one was more clear because things were broken out into commits. The alternative would have been separate PRs for things like "only build Travis against master", "drop unused Firefox Karma launcher", &c., which on balance would have just added noise - a single PR for these build config updates seemed like the best way to structure it.

[taion]

All right, what do we need to move this forward? Do you just want the Karma config to line up to the old one?

We're having people on Discord complain about how they can't debug in Karma right now because the code is mangled with the coverage instrumentation, so I'd like to move this forward for the benefit of other potential contributors.

[mjackson]

Do you just want the Karma config to line up to the old one?

Yep. To quote myself earlier:

Please narrow down this PR to only include the coverage fixes.

[taion]

Closing and splitting up this PR.