Remove side effects from stryker plugins

[nicojs]

Right now it is quite difficult to to tryout 1 plugin by linking it in your project (npm i file:./path/to/plugin). This is because plugins work based on global side effects.

For example: foo-transpiler. The index.js file would look like:

const { TranspilerFactory } = require('stryker-api/transpile');
const FooTranspiler = /*...*/;
TranspilerFactory.instance().register('foo', FooTranspiler);

The problem is that the instance of require('stryker-api/transpile'); is different in the linked package. This is by design (standard nodejs require functionality).

We can solve this issue by not relying on side effects. Instead plugins would export:

const FooTranspiler = /*...*/
exports.FooTranspiler = FooTranspiler;

/* OR in typescript: */
export { FooTranspiler } from './FooTranspiler';

@simondel what do you think?

The magic will be the postfix. This could be one of these: ConfigEditor, Transpiler, Runner, Framework, Mutator.

If a plugin uses one of the factories we should create a deprecation warning. We can remove the factories in the 1.0 release.

UPDATE:

Lot of work is done. Still todo:

• Replace calls to getLogger to DI
• Deprecate old api (using process.emitWarning): https://nodejs.org/dist/latest-v11.x/docs/api/process.html#process_process_emitwarning_warning_type_code_ctor
• Log deprecated message when loading config via DI (using process.emitWarning): https://nodejs.org/dist/latest-v11.x/docs/api/process.html#process_process_emitwarning_warning_type_code_ctor

[nicojs]

This is actually also the reason that people cannot simply use npm i ../stryker/packages/stryker-webpack-transpiler to install an altered plugin.

[simondel]

How would Stryker know what is exported? What does require return?

[nicojs]

In order to understand exactly what happens in NodeJS land, it makes sense to look at how node modules are loaded.

The text of a javascript file being required is actually just placed in a wrapper function. The wrapper function looks like this: (from https://nodejs.org/api/modules.html#modules_the_module_wrapper)

(function(exports, require, module, __filename, __dirname) {
// Module code actually lives in here
});

Both exports and module can be used to export stuff from the module. To illustrate what happens (from https://nodejs.org/api/modules.html#modules_exports_shortcut):

  const module = { exports: {} };
  const { require, filename, dirname } = /*...*/;
  ((exports, require, module, __filename, __dirname) => {
   // Module code actually lives in here
  })(module.exports, require, module, filename, dirname);

So exports is just a convenience parameter of the wrapper function. This is just confusing IMHO. This is why exports = class Transpiler will not have any effect, because you're just reassigning a local parameter value, but module.exports = class Transpiler does have the expected effect.

The JS value that is on module.exports after executing the node module is what the return value will be of require('./foo').

Back to the proposition of this issue. Doing this in typescript:

// index.ts (FooTranspiler project)
export { FooTranspiler } from './FooTranspiler';

Translates to this javascript:

// index.js (FooTranspiler project)
var FooTranspiler_1 = require("./FooTranspiler");
exports.FooTranspiler = FooTranspiler_1.FooTranspiler;

Which in turn means that require will contain an object with the FooTranspiler property like this:

const plugin = require('./FooTranspiler');
plugin.FooTranspiler // <== this is the FooTranspiler.

Other ideas may include a karma like model:

// From: https://github.com/karma-runner/karma-mocha/blob/master/lib/index.js
module.exports = {
  'framework:mocha': ['factory', initMocha]
}

This is really flexible, but also complicated. For karma it ties into there dependency injection mechanism. So in this example the array ['factory', ...] is just delivered to the dep injection library (https://www.npmjs.com/package/di if i'm not mistaken).

Or we can do something clever with the name property (automatically set for all functions, including ES6 classes).

What will it be?

[simondel]

Right, and because we can read the keys of the properties on the object that require returns, we can determine the plugin types that a plugin exposes. Am I correct in thinking this?

[nicojs]

Exactly. Eliminating the side effects. This will allow us to just npm link (or npm install with npm v5) a plugin right into any project to start debugging. We might even be able to remove the peer dependency on stryker-api, leaving it as dev dependencies (need to think about that more, out of scope for this issue).

So, you're in?

[simondel]

Let's do this! (After we squat some of the bugs we have right now 🐛)

[nicojs]

Wait! We have bugs? 😲

[nicojs]

Unfortunately this issue got a bit more complicated with the introduction of the logging api (#954). As calling setImplementation also introduces side effects (d'uh). The solution is to inject the getLogger method into the plugins.

We could do this this by introducing a getLogger method on every api interface method or by using a dependency injection framework.

Personally, I'm now more gravitating towards a dependency injection framework to prevent the api's being cluttered with this stuff.

@stryker-mutator/stryker-core what are your thoughts on this?

A "dependency injection framework" sounds pretty heavy. We could create a very light implementation using Destructing assignments.

It would look roughly like this:

interface DependencyContainer { // this could reside in "stryker-api" somewhere
   getLogger: LoggerFactory;
   strykerOptions: StrykerOptions;
   // add more stuff that can be injected
}

// Within a TestRunner plugin
class MyAwesomeTestRunner {
  private readonly log: Logger;

  constructor(settings: TestRunnerSettings, { getLogger }: DependencyContainer ) { 
     this.log = getLogger(MyAwesomeTestRunner.name);
  }
}

// Within Stryker:
const dependencies: DependencyContainer = createDependencyContainer();
const settings = createTestRunnerSettings(); // Our normal test runner settings, unchanged
new TestRunner(settings, dependencies);

After this, it should be fairly straightforward to remove all implementations from the stryker-api, leaving only pure Interfaces. We will never have problems again and can even remove stryker-api from the peerDependencies if we want.

[nicojs]

Side note: this will make it much easier to run Stryker on the Stryker modules themselves! As we can simply use local references to plugins.

[simondel]

Constructor injection of dependencies seems like a good way to go. I don't really like the idea of an object that contains our dependencies and injecting that

[nicojs]

Well.. maybe. You mean, just use the one and only settings object as a "Dependency injection container". That would work, is unpractical. The idea of the dependency injection container is that it would be constructed on the child process side. The current settings objects are pojo's to be serialized and send over the wire.

Using that settings object to also house our getLogger method would mean constructing it without the getLogger function, send it to the child process, enhancing it with getLogger before injecting it into the constructor.