Add typecheck as tests. #134
Conversation
`npm install --save-dev typescript @types/node`
There was a problem hiding this comment.
It looks like this PR doesn't achieve its objectives. It should be reworked.
The added file test_types.ts is supposed to be a smoke test. It doesn't use the existing test harness. That's totally fine. I suggest moving it to a separate directory, e.g., ts-tests, and adding a different command to run it, e.g., npm run ts-test. Then I can update the GitHub action to run it automatically.
package.json
Outdated
| }, | ||
| "scripts": { | ||
| "pretest": "tsc", |
There was a problem hiding this comment.
I am not an expert in Typescript, but I don't see any file produced running this command.
There was a problem hiding this comment.
Yes, intentionally omitted compiling. Just using TypeScript for the typechecking in this PR. noEmit option makes the behavior.
tests/test_types.ts
Outdated
| } | ||
|
|
||
| test_execTypes() | ||
| test_matchTypes() |
There was a problem hiding this comment.
I don't think this file is ever executed. I added console.log() and I couldn't see its output. I tried to run it with ts-node and it fails to compile (some import problems).
There was a problem hiding this comment.
Yes. These code won't run. However TypeScript checks them with tsc. Removing them makes following error with current config.
❯ npx tsc
tests/test_types.ts:16:10 - error TS6133: 'test_matchTypes' is declared but its value is never read.
16 function test_matchTypes() {
~~~~~~~~~~~~~~~
Found 1 error in tests/test_types.ts:16
|
Thanks for your review!
How about this change? e380c18 |
|
Thank you, I plan to review it in the coming days. |
|
The code is in https://github.com/uhop/node-re2/tree/kachick-add-ts-tests branch. It includes the minimal fix from #141. It fails to compile with the latest Typescript. I am not sure how to handle a type expansion in TS correctly. I am proposing a way in my next commit to this branch. Please review because I am not a TS programmer and can make stupid mistakes. ;-) |
|
Moving the development to #142. |
This PR based on #133
When I digging above issue, I found #101 (comment)
So tried to add typecheckers. Similar ways are used in https://github.com/octokit/types.ts/blob/725c32e3a3827d8e89a232fc09c7225fdaea84e0/test.ts.