Skip to content
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

Allow a custom build command, instead of the transpiler plugin #2276

Closed
nicojs opened this issue Jun 27, 2020 · 1 comment · Fixed by #2292
Closed

Allow a custom build command, instead of the transpiler plugin #2276

nicojs opened this issue Jun 27, 2020 · 1 comment · Fixed by #2292
Labels
🚀 Feature request New feature request

Comments

@nicojs
Copy link
Member

nicojs commented Jun 27, 2020

This issue is a continuation of the discussion started in #1514 (comment)

Transpiling code in a mutation switching world...

Example project

┣ src
┃ ┣ foo.ts
┃ ┗ tsconfig.json
┣ test
┃ ┣ foo.spec.ts
┃ ┗ tsconfig.json
┗ tsconfig.json

Before mutation switching, a user would configure the transpiler plugin (for example transpilers: ["typescript"]). This would result in the code being transpiled in-place and in-memory.
Results were written to the sandbox:

┣ .stryker-tmp/sandbox-1234
┃               ┣ src
┃               ┃ ┗ foo.js
┃               ┗ test
┃                 ┗ foo.spec.js
┣ src
┃ ┣ foo.ts
┃ ┗ tsconfig.json
┣ test
┃ ┣ foo.spec.ts
┃ ┗ tsconfig.json
┗ tsconfig.json

Now, with mutation switching, we want to let users configure a custom build command. This way we don't need to support all different kinds of transpilers.

Example:

┣ .stryker-tmp/sandbox-1234
┃               ┣ src
┃               ┃ ┣ *foo.ts
┃               ┃ ┗ tsconfig.json 
┃               ┣ test
┃               ┃ ┣ foo.spec.ts
┃               ┃ ┗ tsconfig.json
┃               ┗ tsconfig.json
┣ src
┃ ┣ foo.ts
┃ ┗ tsconfig.json
┣ test
┃ ┣ foo.spec.ts
┃ ┗ tsconfig.json
┗ tsconfig.json

(*foo.ts is instrumented here)

The build command configured will run inside the sandbox-1234. The advantage here is a massive decrease of maintainance for the Stryker team. We no longer need to support all different kinds of transpilers (we're currently supporting webpack, typescript and babel)

However, 2 challenges remain with this approach:

  1. Because of mutation switching,foo.ts will have type errors, and compiling it with the tsc would not result in js output.
  2. All tsconfig.json's might have references outside the sandbox and would not compile. For example "extends": "../../tsconfig.settings.json".

Dealing with (typescript, or eslint) errors

For the first challenge, there is a solution. This documented in microsoft/TypeScript#29651 (comment).
Basically we would have to add // @ts-nocheck to each and every ts/js file (also js because of --checkJS mode). However, I'm hesitant to always add it.
This might not be expected for end-users.

Note: even without a build command configured, we might need to add // @ts-nocheck. For example, the karma-webpack plugin won't work with type errors.

Dealing with references to config files

For the second issue, the solution would be to change the contents of said tsconfig file. "../../tsconfig.settings.json" could be transformed to be "../../../../tsconfig.settings.json. However, there are a few things to consider:

  • There is no way to know if a .json file is a typescript config file, other than educated guesses. People will need to configure one of more tsconfig files, so we know which one to transform.
  • There is no way to transform a tsconfig file without decent jsonc parser. TypeScript comes bundled with such a parser (using ts.parseConfigFileTextToJson). Seeing as projects with typescript will have typescript already installed, I think we can safely require typescript without needing a (peer)dependency on from core.

With the current planned @stryker-mutator/typescript-checker plugin users already configure the tsconfig.json file in typescriptChecker.tsconfigFile.

Note: this problem can also occur for babel. If you add a babel.config.js file, where you require from the root for example.

We could also opt to not transform config files, and instead implement #2163 sooner. I don't like that personally.

An alternative might be to let users do this fixup of config files themselves. For example, by splitting the instrumenting process from the mutation testing process. So this scenario would look like:

  1. User runs npx stryker instrument
  2. User cd's into the .stryker-tmp/sandbox-123 directory and manually changes the config files
  3. User runs mutation testing with npx stryker continue.

I don't like this approach either, personally.

We could of course also decide to keep the current transpiler plugin and accept the maintenance that comes along with it.

@hugo-vrijswijk @simondel @kmdrGroch I'm curious to hear your thoughts on the matter.

@nicojs nicojs added the 🚀 Feature request New feature request label Jun 27, 2020
@nicojs
Copy link
Member Author

nicojs commented Jun 30, 2020

Discussed in-person with @hugo-vrijswijk and @simondel. These are the conclusions

For issue 1: add a header to each js-like file which disables all linters/checkers. For example:

// @ts-nocheck
/* eslint-disable */

We'll probably make it non-configurable and not-optional for now.

For issue 2:
Implement a custom solution for typescript where we allow preprocessing for tsconfig.json files you configure. We'll re-introduce the tsconfigFile option for this and use ts.parseConfigFileTextToJson when the config file exists (it makes sense that typescript is installed as well in that case). We'll also update the @stryker-mutator/typescript-checker plugin to use this same setting so that users won't have to configure 2 tsconfig files.

nicojs added a commit that referenced this issue Jul 4, 2020
Add an ignore header to each js (or friend) file in the sandbox. This should allow files to be transpiled no matter the contents. For more info, see #2276

The header now consists of:

```js
/* eslint-disable */
// @ts-nocheck
```

But we can later decide to include more ignore rules.
nicojs added a commit that referenced this issue Jul 4, 2020
Add an ignore header to each js (or friend) file in the sandbox. This should allow files to be transpiled no matter the contents. For more info, see #2276

The header now consists of:

```js
/* eslint-disable */
// @ts-nocheck
```

But we can later decide to include more ignore rules.
nicojs added a commit that referenced this issue Jul 5, 2020
Add a new `tsconfigFile` option (reused from the old `@stryker-mutator/typescript` package). Use this new option in the `@stryker-mutator/typescript-checker` instead of a specific typescript-checker specific one

From stryker core, use this setting to rewrite `tsconfig.json` file's `references` and `extends` when they refer config files that fall outside of the sandbox.

For example:

```json
{
  "extends": "../../tsconfig.settings.json",
  "references": {
     "path": "../model"
  }
}
```

becomes:
```json
{
  "extends": "../../../../tsconfig.settings.json",
  "references": {
     "path": "../../../model"
  }
}
```



The implementation relies on typescript being available, but will only import it when it found a tsconfig.json file. Once a tsconfig file is found, referenced files within the sandbox also get rewritten.

Closes #2276
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 Feature request New feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant