This typings #40

zoehneto · 2018-03-23T18:47:16Z

Optionally applies this types to functions. Fixes #33 .

…ctly yet

coveralls · 2018-03-23T18:49:03Z

Coverage increased (+0.3%) to 85.649% when pulling 641f3e5 on zoehneto:this-typings into 7d9f39a on urish:master.

urish · 2018-03-23T23:10:52Z

package.json

  "scripts": {
    "build": "rimraf dist && tsc && copyfiles -u 1 src/type-collector-snippet.ts dist",
-    "format": "prettier --write src/**/*.ts packages/**/src/**.ts",
+    "format": "prettier --write src/*.ts src/**/*.ts packages/**/src/**.ts",


Any reason for adding src/*.ts? afaik src/**/*.ts also includes any .ts files directly under src

At least for me (on Arch Linux) it is required or the files directly under src are ignored.

urish · 2018-03-23T23:13:35Z

src/apply-types.ts

            suffix = ')';
        }
+        if (opts && opts.comma) {
+            suffix = ', ';


are we sure opts.comma and opts.parens are mutually exclusive? Otherwise, we need to account for the case where both opts.comma and opts.parens set suffix

opts.parens is only used for one parameter arrow functions so this should never happen since arrow functions use the surrounding this and therefore don't need an explicit this type.

urish · 2018-03-23T23:18:42Z

src/apply-types.ts

+        }
        replacements.push(Replacement.insert(pos, ': ' + prefix + sortedTypes.join('|') + suffix));
+        if (opts && opts.thisType) {
+            replacements.push(Replacement.insert(pos, 'this'));


Since we add two replacement at the same position, let's use the third priority parameter for Replacement.insert to guarantee that this will always come before the colon. Or we can just combine both into one replacement - similar to what with do with prefix and suffix, we can add another variable called thisPrefix or similar.

I added a thisPrefix variable in the latest commit.

urish · 2018-03-23T23:21:39Z

src/integration.spec.ts

+        `);
+    });
+
+    it('should not add `this` type', () => {


why is should not add this this here? Let's change the test description to explain that (e.g. "it should not add this type in case it can be inferred")

Expanded the explanation.

urish · 2018-03-23T23:22:45Z

src/type-collector-snippet.ts

    filename: string;
    pos: number;
-    opts: any;
+    opts: ICollectedTypeInfo;


Nice! Though, I think it will make more sense to go the other way around - move the definition of ICollectedTypeInfo here, and import it in apply-types.

Moved the type to type-collector-snippet.ts.

urish · 2018-03-23T23:23:45Z

Lovely, thank you very much! I left a few comments, but overall looks really nice :)

zoehneto · 2018-03-24T09:19:08Z

Thanks for the quick review, I addressed your comments in the latest commit.

urish · 2018-03-24T09:39:36Z

Awesome, thanks!

Just out of curiosity - have you used TypeWiz on any of your projects?

zoehneto · 2018-03-24T09:40:27Z

What is not yet covered by this PR (and was also not covered in #27) is how to roll out these changes to typewiz-node and typewiz-webpack. I personally don't use either since I bundle with rollup and then test the bundle but this should be considered going forward.

zoehneto · 2018-03-24T09:42:20Z

Just saw your comment, I have experimentally added typewiz to rtf.js where I use a custom script for instrumentation.

urish · 2018-03-24T09:49:28Z

Very interesting - so in this case, you are basically writing an instrumented copy of your source code to the build directory, and then the tests run using the instrumented source code, right?

I couldn't find where you told the bundle / test steps to read the source files from the build directory though, can you please point me?

zoehneto · 2018-03-24T10:29:00Z

It works as follows:

typewiz-instrument.js instruments the files using the typewiz api (the key here is to pass the original filenames in the source directory to the instrumenter), compiles the individual files using the typescript api and then writes the files to the build directory.
Then npm run bundle is invoked which creates three UMD / ES5 bundles in the dist directory based on the instrumented code from build
The test suite is started using npm run test which runs the script test/test-rtfjs.js (this script executes a test for every directory under test/test-files and applies the collected types after all tests are finished). To execute the individual tests test/utils.js is used which converts rtf files to html. Since rtf.js requires a browser to run (because of some of the libraries it's built on) the actual test is executed using JSDOM which loads the scripts from dist (line 30 - 32) and has to be passed $_$twiz (line 105).

urish · 2018-03-24T10:45:14Z

Got it, thanks!

I'm currently helping migrate immer to TS using typewiz, and they are also using rollup for their build. In their case, I used rollup-plugin-typescript2. In your build, you are running TypeScript as a separate compile step. Is there any reason for preferring running TypeScript separately and not as part of rollup?

zoehneto · 2018-03-24T11:14:34Z

In rtf.js we build three separate bundles (for rtf.js, emf.js and wmf.js) but building rtf.js requires building the other two with tsc in the same step (because of some import tricks it uses) so if it used the rollup plugin emf.js and wmf.js would be built twice.

urish · 2018-03-24T11:20:15Z

And how useful have you found TypeWiz so far for rtf.js?

zoehneto · 2018-03-24T11:28:18Z

Sadly I found your blog post too late. A month ago it was just three plain ES5 files with around 9000 lines of code and then I did the entire conversion by hand and added all the typings :(

urish · 2018-03-24T14:45:19Z

I hear you... I did the same with several projects before coming up with TypeWiz. But the future will be brighter :)

urish · 2018-03-26T10:05:42Z

Released as part of 0.7.0

zoehneto added 3 commits March 22, 2018 15:32

Extract createProgram from applyTypes to allow sharing with instrument

6f7441d

Initial implementation, checking for implicit this doesn't work corre…

a1fc415

…ctly yet

Check implicit this with diagnostic

f0242e4

Fix body detection and improve test coverage

ed0f4e1

urish suggested changes Mar 23, 2018

View reviewed changes

urish added the enhancement New feature or request label Mar 23, 2018

Address review comments

641f3e5

urish merged commit 8e227d0 into mockdeep:master Mar 24, 2018

urish mentioned this pull request Apr 1, 2018

CLI [RFC] #53

Closed

This typings #40

This typings #40

Uh oh!

Conversation

zoehneto commented Mar 23, 2018

Uh oh!

coveralls commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

urish commented Mar 23, 2018

Uh oh!

zoehneto commented Mar 24, 2018

Uh oh!

urish commented Mar 24, 2018

Uh oh!

zoehneto commented Mar 24, 2018

Uh oh!

zoehneto commented Mar 24, 2018

Uh oh!

urish commented Mar 24, 2018

Uh oh!

zoehneto commented Mar 24, 2018

Uh oh!

urish commented Mar 24, 2018

Uh oh!

zoehneto commented Mar 24, 2018

Uh oh!

urish commented Mar 24, 2018

Uh oh!

zoehneto commented Mar 24, 2018

Uh oh!

urish commented Mar 24, 2018

Uh oh!

urish commented Mar 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Mar 23, 2018 •

edited

Loading