Calling config.set asynchronously not working

[kahnvex]

I'm trying to call config.set asynchronously in my configuration function. Doing this because we use a tool that finds an available port asynchronously, so we don't have all the configuration at the time the call to our configuration function is made. It appears as though the function never gets called.

[kahnvex]

To demonstrate this, you can do this:

'use strict';


module.exports = function(config) {
  var configuration = {...};
  setTimeout(config.set.bind(config), 10, [configuration]);
};

[kahnvex]

It looks like this isn't really a bug, and is just bad design.

The config.set function simply replaces object properties on the configuration object. It does not notify karma when configuration is finalized, therefore, config.set must complete execution before the configuration function returns. This just begs the question, why on Earth would one write it this way? It would be much clearer to require the user to return the new configuration.

In any case I'm fairly sure changing this will break backwards compatibility. So if there is an improvement here, it will probably need to go in the next release.

[maksimr]

@kahnjw configuration step is not asynchronous. I think this was made because it covers most cases and it is simple.
Perhaps we should allow asynchronous configuration this requires discussion.

Now karma Itself try find free, available port you can try using it or you can run special script before running karma and export port to environment.

Something like this:

getFreeAvailablePort.sh && karma start

or run karma programmatically from node script how it do karma-grunt.

The config.set function simply replaces object properties on the configuration object. It does not notify karma when configuration is finalized, therefore, config.set must complete execution before the configuration function returns. This just begs the question, why on Earth would one write it this way? It would be much clearer to require the user to return the new configuration.

Really I don't see problem with function set.
We pass to function instance of configuration object and user simple extend configuration. User doesn't worry about how create configuration object.

In any case I'm fairly sure changing this will break backwards compatibility. So if there is an improvement here, it will probably need to go in the next release.

PR welcome

Thanks!

[kahnvex]

Really I don't see problem with function set.
We pass to function instance of configuration object and user simple extend configuration. User doesn't worry about how create configuration object.

The way it is currently we give the user a callback to extend configuration with. That is fine, but in javascript, when given a callback, one expects it will behave the same whether called synchronously or asynchronously. If the goal is to require a synchronous extend, for whatever reason, it would be more clear to require something like this:

module.exports = function(config) {
  return config.extend({
    my: 'overrides'
  });
};

Making the user return the extended object makes it clear that this must be done synchronously. I say this because I spent about 30 minutes trying to figure this out, assuming that config.set could be called asynchronously.

PR welcome

I'd like to help out with this and I'll be looking into making it async today.

Thanks.

[cayblood]

@kahnjw Any progress on making it async?

[larrymyers]

I'd suggest going the route @maksimr has suggested and starting karma programmatically. That way you can do all of your async setup first, and then as a final step pass the constructed config to karma.

It does invert things compared to using the cli, but it will work fine.

[christophmegusta]

i am having same problem, but not for port checking but rather parsing some environment configs with shell-source which only works async.
so our webpack config is actually a promise but karma does not accept that.

[spencerwilson-optimizely]

changing this will break backwards compatibility

Would it? It seems like currently, JS Karma config files export plain objects. They could be backward-compatibly distinguished from thenables (terminology from here) by the presence of a then function-valued property. Supposing it's safe to say that no function-valued then properties might currently be used for anything...

[kahnvex]

Wow this was a long time ago. IIRC it would break compatibility if consumers are inadvertently depending on an asynchronous config.set call effectively being a noop. If you're strictly adhering to semver it's a major bump but I don't think anyone is that extreme. A minor version bump is definitely necessary though. All assuming Karma follows semver (might not though 🤷).

Not sure what this has to do with thenables/promises/whatever. If a thenable were added to achieve the asynchrony through a different interface, you'd still need a minor semver bump so it's sort of all the same I guess.

[spencerwilson-optimizely]

if consumers are inadvertently depending on an asynchronous config.set call effectively being a noop

Ah, hadn't considered that. Feels very 'edge case'-y to me. Like you experienced back in 2014 (!), I suspect people have must-have information in config.set calls, and they'd notice very keenly if they weren't happening.

If a thenable were added to achieve the asynchrony through a different interface, you'd still need a minor semver bump so it's sort of all the same I guess.

Yep. To clarify, my previous post was contemplating making the karma.config.js interface be "a CommonJS module whose export is either a plain object or a thenable that fulfills to a plain object".

[usergenic]

This sure would simplify things for karma configurations involving starting upstreamProxy and finding available ports etc. Right now I am finding myself writing wrapper CLI for karma to coordinate upstreamProxy servers and karma server starts so I don't have to include boilerplate scripts in all my projects that need them. If I could just make the exported function that calls config.set() return a promise, that would me wonderful.

[zhangliangyi]

Any update on this issue? I am having the same problem, as the karma configuration in our project is generated asynchronously. I would like to assign the async configuration object to the configuration object in karma.conf.js to allow other developer to override it.

module.exports = async function (config) {
  const karmaConfig = await getKarmaConfig("dev");
  
  config.set({
    ...karmaConfig
  });

  return config;
};

[niksy]

Is there any movement for this? What can we do to help with this?

[brimworks]

Also running into this issue.

I plan to "hack" around this by using child_process.spawnSync() to force process to block... not an optimal solution :(.

[devoto13]

The feature boils down to adding await on this line, but because this method is a public API and because of the poisonous nature of asynchronicity it is not so trivial to implement. In any case, I'm willing to review a PR if somebody comes up with a reasonable implementation.

[brimworks]

@devoto13 , I was actually looking at putting together a PR, and I realized the trickle-down effect which would impact the public new Server() API (which would need to return a Promise<Server>)... and I figured that such an incompatible change would be rejected. Specifically, this code would also need an await:

https://github.com/karma-runner/karma/blob/master/lib/server.js#L68

Also, I think you mean this line (maybe a recent change on master branch changed the line number?):

https://github.com/karma-runner/karma/blob/master/lib/config.js#L412

If you have suggestions on how to keep the API compatible (or are willing to break API compatibility), then putting together a PR shouldn't be difficult. Perhaps just moving the config parsing into the Server.start() method (which is already async) would be a step toward backward compatibility? ...however, could that result in a behavior change that others are depending on?

Thanks!

[devoto13]

Perhaps just moving the config parsing into the Server.start() method (which is already async) would be a step toward backward compatibility?

Unfortunately, this is not enough as there are other places, where parseConfig is used. This also will be a significant behavior change for the people using the Karma programmatically as config parsing, logger setup, and DI configuration will happen at a different time. Besides that, parseConfig function itself is part of the public API, so it can't change in the minor release. And the last point: it is not enough to only update the server, because this function is also used by runner and stopper, which are also part of the public API.

I did a bit of thinking on the backward compatible design. This way we can land this feature in the minor release and then perform a breaking cleanup in the next major. Let me know what you think.

1. Add a new flag to parseConfig (similar to feat(config): improve karma.config.parseConfig error handling #3635). If the flag is passed, then the method returns Promise<Config> instead of returning Config synchronously. Print a deprecation warning if the flag is not passed. This way parseConfig stays backward compatible.
2. Update Server to accept either Config object (i.e. result of the parseConfig() call) or cliOptions (current).
   • In a former case it gets the parsed Config object and does not need to do any async parsing.
   • In a later case execute the same synchronous call to parseConfig() as it does right now. This way programmatic users of new Server() are not broken.
3. Parse config asynchronously in the cli.js and pass a parsed Config object to the Server class. This way Karma itself does not use a deprecated behavior.
4. (*) We will probably need to move logger.setupFromConfig call/design a new public API for configuring logging outside of the Server. Need to think a bit more about it.
5. Update runner.js and stopper.js to also pass a flag to parseConfig() to support async karma.conf.js.

In the next major we can clean up the flag and turn this behavior on by default. This will break public API consumers, but hopefully, they are already updated by the time because of a warning.

Does this sound reasonable?

[karmarunnerbot]

🎉 This issue has been resolved in version 6.3.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀