Closed nicojs closed 5 years ago
This is actually also the reason that people cannot simply use npm i ../stryker/packages/stryker-webpack-transpiler
to install an altered plugin.
How would Stryker know what is exported? What does require return?
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?
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?
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?
Let's do this! (After we squat some of the bugs we have right now :bug:)
Wait! We have bugs? :astonished:
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.
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.
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
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.
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: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:
@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:
getLogger
to DIprocess.emitWarning
): https://nodejs.org/dist/latest-v11.x/docs/api/process.html#process_process_emitwarning_warning_type_code_ctorconfig
via DI (usingprocess.emitWarning
): https://nodejs.org/dist/latest-v11.x/docs/api/process.html#process_process_emitwarning_warning_type_code_ctor