feat(package): restructure package internals #2714

nicojs · 2021-01-25T20:45:42Z

Redirect typescript output to "dist" directories
No longer compile "api" package root ts files (reduce complexity)
Enable typescript --strict mode
Fix 2 (small) bugs that resulted from uninitialized properties
Use default import syntax for commonjs modules instead of wildcard import syntax (eg. ~~import * as path from 'path'~~ `import path from 'path'),
Use named exports instead of a mix of default exports and named exports everywhere.
Force named exports using an eslint rule.

If you're using deep-imports from @stryker-mutator/core or friends, they will have to be updated. Note: as a general rule: don't rely on deep imports from Stryker, as they might break at any moment.

- import { default as HtmlReporter } from '@stryker-mutator/core/src/reporters/html/html-reporter';
+ import { HtmlReporter } from '@stryker-mutator/core/dist/src/reporters/html/html-reporter';

Update all package to direct TS output to dist directories in each package.

Reduce complexity by no longer compiling files in the root of the api package.

bartekleon · 2021-01-27T08:29:19Z

Is it ready to be checked @nicojs ?

nicojs · 2021-01-27T08:30:35Z

@kmdrGroch this is ready for review now. You might want to focus on the eslint and typescript config stuff, nearly all files have changes 🤷‍♀️

bartekleon · 2021-01-27T08:33:21Z

packages/api/check.js

@@ -0,0 +1 @@
+module.exports = require("./dist/src/check")


Should JS and .D.TS files be on github tho? 🤔

Great question!

I would actually like to have it as a *.ts file, but that would mean we would need to compile those files in place (not to the dist directory), because I want this import to work: import { File } from '@stryker-mutator/api/core' and that can only be done with a file called "core.js" inside api (on node < 12.7).

When we drop support for node < 12.7 we can start using exports and remove these root files altogether.

I don't want to make an exception for some files to be compiled in place, because that also means making an exception in our clean script and making the output files hidden in vscode config (i don't like hidden files 😢). This all adds complexity that I don't want.

Do you agree with this? In short: we should be able to remove these files once we can use exports in package.json.

bartekleon · 2021-01-27T08:33:50Z

packages/api/core.ts

@@ -1,11 +0,0 @@
-export { default as File } from './src/core/file';


typescript files are removed? 🤔

Yes, see my previous remark.

bartekleon · 2021-01-27T08:37:09Z

packages/api/test/unit/core/file.spec.ts

@@ -1,7 +1,7 @@
 import { expect } from 'chai';
 import { deserialize, serialize } from 'surrial';

-import { File } from '../../../core';
+import { File } from '../../../src/core';


I don't quite like pathing to src from tests / packages

Ah, you mean just import from '../../../core'? Yes that would be possible here because of the new d.ts files.

sorry, that doesn't work. See my latest comment

bartekleon · 2021-01-27T08:38:38Z

packages/core/package.json

@@ -2,13 +2,13 @@
  "name": "@stryker-mutator/core",
  "version": "4.4.0",
  "description": "The extendable JavaScript mutation testing framework",
-  "main": "src/index.js",
+  "main": "dist/src/index.js",


dist/src/index? so is it dist or src ;)

Yes. So src is compiled to dist/src and test is compiled to dist/test. If you really want src to be compiled to dist we would need to add the tests inside the src directory. I don't really mind this that much; we can go either way.

What do you think?

I haven't noticed we have src and test in dist. If it is like this, it is ok i believe :)
Tho it should be consistent 🤔

What would be consistent?

I've been thinking and having src compile to dist would also mean moving the typings, src-generated, and schema directories src directory. We can do that, but it would take some more effort.

bartekleon · 2021-01-27T08:39:32Z

packages/core/src/child-proxy/child-process-proxy.ts

  public readonly proxy: Promisified<T>;

-  private readonly worker: ChildProcess;
+  private readonly worker: childProcess.ChildProcess;


I like this change, provides more information what it is, especially in fork

bartekleon · 2021-01-27T08:51:14Z

packages/jest-runner/src/messaging.ts

@@ -8,14 +8,14 @@ class State {
  }

  public coverageAnalysis: CoverageAnalysis = 'off';
-  private mutantCoverageHandler: MutantCoverageHandler;
+  private mutantCoverageHandler?: MutantCoverageHandler;


again optional? 🤔

Yeah, the same reasoning as before. This is because I enabled --strictPropertyInitialization.

bartekleon · 2021-01-27T08:53:26Z

packages/mocha-runner/test/unit/mocha-test-runner.spec.ts

@@ -1,5 +1,5 @@
 import { expect } from 'chai';
-import * as Mocha from 'mocha';
+import Mocha from 'mocha';


Mocha? not mocha?

It's a constructor function (or class) 🤷‍♂️

bartekleon · 2021-01-27T08:53:58Z

packages/test-helpers/src/test-injector.ts

-  public options: StrykerOptions;
-  public logger: sinon.SinonStubbedInstance<Logger>;
-  public injector: Injector<PluginContext>;
+  public pluginResolver!: sinon.SinonStubbedInstance<PluginResolver>;


!? are they possibly undefined?

Yes, this is because of --strictPropertyInitialization again. But this time I used a ! (not null assertion) instead of a ?, because I know for sure this can only theoretically be undefined and will always be defined in practice because I call reset directly inside this file (and it's a singleton).

bartekleon · 2021-01-27T08:55:11Z

perf/tasks/install.ts

+import fs from 'fs';
+import minimatch from 'minimatch';;
+import path from 'path'
+import execa from 'execa';;


what happened here? 😆

Midnight hacking 😅

Will fix

bartekleon · 2021-01-27T08:55:23Z

perf/tasks/run-perf-tests.ts

-import * as execa from 'execa';
+import fs from 'fs';
+import path from 'path'
+import execa from 'execa';;


api/*.ts files should be removed.

nicojs · 2021-01-27T11:16:49Z

Thanks for your great feedback @kmdrGroch . I've commented on each remark and fixed some issues.

bartekleon · 2021-01-27T12:03:09Z

The only thing thats left is just making it green again :D

bartekleon · 2021-01-27T12:03:40Z

e2e/test/exit-prematurely-dry-run-fails/verify/verify.ts

 import chai, { expect } from 'chai';
-import { it } from 'mocha';


I think thats an issue

Importing it from mocha is optional. Mocha will make sure it is globally available. I've removed it since we're never importing it.

nicojs · 2021-01-27T12:06:51Z

@kmdrGroch I had to revert the imports in the api tests.

import { TestStatus } from '../../../src/test-runner'; // -> this works
import { TestStatus } from '../../../../test-runner'; // -> this compiles, but at runtime won't resolve, because the root dir is one directory more up
import { TestStatus } from '../../../../../test-runner'; // -> this would resolve at runtime, but doesn't compile 🤷‍♂️

nicojs · 2021-01-27T12:54:26Z

Good find with the snapshot file. Snapshot remapping broke somehow, I've implemented it with some clever sourcemap magic.

bartekleon · 2021-01-27T14:17:03Z

If you want I can review it once more, but probably tomorrow since today I am busy. Or you could merge it and hope for the best 😅

nicojs · 2021-01-27T16:30:17Z

Living on the edge 😉

If we something after the fact, we will open a new PR.

nicojs added 11 commits January 25, 2021 21:39

refactor(dist): ouput typescript to "dist" dir

dfe8dc2

Update all package to direct TS output to dist directories in each package.

refactor(api): no longer compile api package root ts files

648ea68

Reduce complexity by no longer compiling files in the root of the api package.

Fix core.d.ts file

8f7f4a0

Update stryker bin

4be037b

Add mono schema to clean

be3ca7d

Take new dist dir into account when loading plugins

8d989e1

fix plugin loader test

86de21a

Remove unnesesary timer.reset() call, instead greedy initialize it

0043c21

Fix incorrect dry run timout logging

b255e5a

Fix issue with race condition incorrect clearInterval call.

2ce5228

Enable typescript strict mode

62ea482

nicojs requested a review from bartekleon January 25, 2021 21:48

nicojs changed the title ~~refactor(package): redirect compile output to dist directory~~ feat(package): redirect compile output to dist directory Jan 25, 2021

nicojs added 3 commits January 25, 2021 22:55

update paths in jest-env

90910da

refactor(named exports): named instead of default exports

8c9becf

chore(eslint): enable no-default-export rule

d6e4ee9

nicojs mentioned this pull request Jan 27, 2021

Enable "plugin:@typescript-eslint/recommended-requiring-type-checking" in eslint #2717

Closed

refactor(import syntax): enable ts esModuleInterp

74e68de

nicojs changed the title ~~feat(package): redirect compile output to dist directory~~ feat(package): restructure package internals Jan 27, 2021

bartekleon reviewed Jan 27, 2021

View reviewed changes

nicojs added 6 commits January 27, 2021 10:03

Delete report.ts

b3f1f2a

api/*.ts files should be removed.

Remove duplicate ;;

34d29da

Import from package root in api tests

0399af4

rename child -> childProcess

53d2a84

rename fs -> fsPromises

90dd4b0

enable --esModuleInterop in performance tests

766c414

bartekleon reviewed Jan 27, 2021

View reviewed changes

Revert package import in api tests

4c6a9d9

Reconfigure snap files

34c3640

nicojs merged commit e1711d2 into master Jan 27, 2021

nicojs deleted the refactor/typescript-projects branch January 27, 2021 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(package): restructure package internals #2714

feat(package): restructure package internals #2714

nicojs commented Jan 25, 2021 •

edited

Loading

bartekleon commented Jan 27, 2021

nicojs commented Jan 27, 2021

bartekleon Jan 27, 2021

nicojs Jan 27, 2021 •

edited

Loading

bartekleon Jan 27, 2021

nicojs Jan 27, 2021 •

edited

Loading

bartekleon Jan 27, 2021

nicojs Jan 27, 2021

nicojs Jan 27, 2021

bartekleon Jan 27, 2021

nicojs Jan 27, 2021 •

edited

Loading

bartekleon Jan 27, 2021 •

edited

Loading

nicojs Jan 27, 2021 •

edited

Loading

bartekleon Jan 27, 2021

bartekleon Jan 27, 2021

nicojs Jan 27, 2021

bartekleon Jan 27, 2021

nicojs Jan 27, 2021

bartekleon Jan 27, 2021

nicojs Jan 27, 2021 •

edited

Loading

bartekleon Jan 27, 2021

nicojs Jan 27, 2021

bartekleon Jan 27, 2021

nicojs commented Jan 27, 2021

bartekleon commented Jan 27, 2021

bartekleon Jan 27, 2021

nicojs Jan 27, 2021

nicojs commented Jan 27, 2021

nicojs commented Jan 27, 2021

bartekleon commented Jan 27, 2021

nicojs commented Jan 27, 2021

		@@ -1,11 +0,0 @@
		export { default as File } from './src/core/file';

		import chai, { expect } from 'chai';
		import { it } from 'mocha';

feat(package): restructure package internals #2714

feat(package): restructure package internals #2714

Conversation

nicojs commented Jan 25, 2021 • edited Loading

bartekleon commented Jan 27, 2021

nicojs commented Jan 27, 2021

Choose a reason for hiding this comment

nicojs Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicojs Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicojs Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

bartekleon Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

nicojs Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicojs Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicojs commented Jan 27, 2021

bartekleon commented Jan 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicojs commented Jan 27, 2021

nicojs commented Jan 27, 2021

bartekleon commented Jan 27, 2021

nicojs commented Jan 27, 2021

nicojs commented Jan 25, 2021 •

edited

Loading

nicojs Jan 27, 2021 •

edited

Loading

nicojs Jan 27, 2021 •

edited

Loading

nicojs Jan 27, 2021 •

edited

Loading

bartekleon Jan 27, 2021 •

edited

Loading

nicojs Jan 27, 2021 •

edited

Loading

nicojs Jan 27, 2021 •

edited

Loading