Install @types/node

[JHWelch]

I'm not sure what the state of TypeScript in this repo is, but just when looking at the tests for #139 I noticed that at least the Node types were not there. Added that for easier dev experience.

Not sure if there was any reason there aren't any @types packages installed, but wanted to pull this out of any other work.

[KristjanESPERANTO]

Thanks for the PR! 🙂 Quick question: What specific issues were you experiencing that prompted adding @types/node?

We're using Node.js' native TypeScript support (without a build step), so in my understanding we don't need @types/node. Adding it may cause version conflicts or duplicate type definitions.

[JHWelch]

@KristjanESPERANTO I'm not an expert here, but from my understanding the types/* packages are more involved in static checks then the actual running/compiling of the TypeScript.

You don't need type defintions for running typescript, because at the end of the day it just needs to strip out all the typescript symbols and run the thing as pure JavaScript.

The specific place I'm seeing this is importing the test helpers in my test file

And you can see the same if I run a tsc --noEmit to check.

❯ npx tsc --noEmit
scripts/check-modules/__tests__/index.test.ts:2:20 - error TS2307: Cannot find module 'node:assert/strict' or its corresponding type declarations.

2 import assert from "node:assert/strict";
                     ~~~~~~~~~~~~~~~~~~~~

scripts/check-modules/__tests__/index.test.ts:3:30 - error TS2307: Cannot find module 'node:test' or its corresponding type declarations.

3 import { describe, it } from "node:test";
                               ~~~~~~~~~~~

This is resolved with the package installed.

[JHWelch]

And it might actually make sense to add a Static Type analysis workflow in addition to the other test suites.

If you look at the NodeJS Running TypeScript Natively page it notes that the nodeJS TypeScript loader does not actually use the application's tsconfig.json.

If you want the full type checking, you should still use a tsc --noEmit as a CI step, even if you don't actually use it to compile the TypeScript.

[sdetweil]

If the developer does the tsc, why should it be part of the ci install. Nothing the user can do, and it’s all developer work ahead of time

it could be part of the test cases, again , run by the developer. We expect the module to be complete before posting

[JHWelch]

@sdetweil That is actually what I'm referring to, the types are part of a develop time check, that you could run as a GitHub Workflow on PRs before the code is merged and executed.

Right now this project is using TypeScript, but there is no tooling validating the TypeScript. So the only thing stopping errors from being committed is developers noticing in their IDE.

Consider this contrived example:

// scripts/example.ts

const result: number = "hello";  // Type error: string ≠ number

const arr: string[] = [1, 2, 3];  // Type error: number[] ≠ string[]

function greet(name: string) {
  console.log("Hello " + name);
}
greet(42);  // Type error: number ≠ string

If I were to save this in the repo and run it, it executes without a problem. It also passes the test suite run with node --run test.

❯ node scripts/example.ts
Hello 42

Obviously that is full of TypeScript errors.

If I run a type check with tsc it comes back with the warnings we'd expect TypeScript to protect us against

scripts/example.ts:1:7 - error TS2322: Type 'string' is not assignable to type 'number'.

1 const result: number = "hello";  // Type error: string ≠ number
        ~~~~~~

scripts/example.ts:3:24 - error TS2322: Type 'number' is not assignable to type 'string'.

3 const arr: string[] = [1, 2, 3];  // Type error: number[] ≠ string[]
                         ~

scripts/example.ts:3:27 - error TS2322: Type 'number' is not assignable to type 'string'.

3 const arr: string[] = [1, 2, 3];  // Type error: number[] ≠ string[]
                            ~

scripts/example.ts:3:30 - error TS2322: Type 'number' is not assignable to type 'string'.

3 const arr: string[] = [1, 2, 3];  // Type error: number[] ≠ string[]
                               ~

scripts/example.ts:6:3 - error TS2584: Cannot find name 'console'. Do you need to change your target library? Try changing the 'lib' compiler option to include 'dom'.

6   console.log("Hello " + name);
    ~~~~~~~

scripts/example.ts:8:7 - error TS2345: Argument of type 'number' is not assignable to parameter of type 'string'.

8 greet(42);  // Type error: number ≠ string
        ~~

So this PR might be getting ahead of itself, maybe it is not needed if we're not actually validating the TypeScript in a test step, but this is part of that side of TypeScript.

[sdetweil]

That the developer could run on their gh workflows. We are not validating their code.

[KristjanESPERANTO]

Ah, now I get it. For type-checking that definitely makes sense. Let's merge this.

Side note: Our existing .ts files have // @ts-nocheck at the top, so they currently skip type validation. But for new test files (from #139) this will be useful.

@JHWelch Follow-up question: Should we add tsc --noEmit to our CI pipeline? And would you be interested in gradually removing the @ts-nocheck comments as part of the testing effort? 🙂

@sdetweil Just to clarify: This isn't about validating the modules we analyze – it's about type-checking our own TypeScript code. The @types/node package helps catch errors in our scripts before they get merged.

[JHWelch]

@KristjanESPERANTO Yes I think that makes sense! I was looking to add that and realized the unit tests are not running in CI either, so handled that first in #141 as it is nice to have that in place before I start futzing with TypeScript, but I'll PR that next.