-
Notifications
You must be signed in to change notification settings - Fork 251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Async transpiler plugin support #433
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just some small remarks.
@@ -24,7 +24,7 @@ export default class CoverageInstrumenterTranspiler implements Transpiler { | |||
this.log = getLogger(CoverageInstrumenterTranspiler.name); | |||
} | |||
|
|||
transpile(files: File[]): TranspileResult { | |||
public async transpile(files: File[]): Promise<TranspileResult> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think async
is not needed here. It is only used to mark methods in which you can use await
@@ -74,7 +74,7 @@ export default class MutantTranspiler { | |||
} | |||
} | |||
|
|||
private transpileMutant(mutant: TestableMutant): Promise<TranspileResult> { | |||
private async transpileMutant(mutant: TestableMutant): Promise<TranspileResult> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed here as well
@@ -17,11 +17,11 @@ export default class TranspilerFacade implements Transpiler { | |||
} | |||
} | |||
|
|||
transpile(files: File[]): TranspileResult { | |||
public async transpile(files: File[]): Promise<TranspileResult> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opinion requested about using async
in one of the functions.
} catch (error) { | ||
return this.errorResult(errorToString(error)); | ||
return Promise.resolve(this.errorResult(errorToString(error))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to leave the function async
because otherwise the two return statements have to be a Promise
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine as it is. It looks clean in the resulting JavaScript.
@simondel it can be merged IMO. What commit message should we use? Strictly speaking its a breaking change, but we could also state that the transpiler api is still experimental (no one is creating stryker transpilers at the moment anyway) @Archcry great job on this PR! |
👍 |
@Archcry Nice job! |
In order to add webpack support in Stryker, Stryker has to support async transpiler plugins.
This pull-request will: