TypeScript type definitions are broken

[harrisgilliam]

This TS code will not build:

import path from 'path';
import { readFileSync } from 'node:fs';
import { addSchema } from '@hyperjump/json-schema/draft-2020-12';
import { compile } from '@hyperjump/json-schema/experimental';

async function main(): Promise<void> {
  const fullPath = path.resolve('./test.schema.json');

  const schema = JSON.parse(readFromFile(fullPath));

  addSchema(schema, 'https://example.com/test.schema.json');

  const compiledSchema = await compile('https://example.com/test.schema.json');

  console.log(JSON.stringify(compiledSchema, undefined, 2));
}

function readFromFile(filename: string): string {
  return readFileSync(filename, { encoding: 'utf8' });
}

main()
  .then(() => {
    process.exit(0);
  })
  .catch((error) => {
    console.log(error);
    process.exit(-1);
  });

It produces the errors:

> tsc

node_modules/@hyperjump/json-schema/lib/invalid-schema-error.d.ts:1:15 - error TS2305: Module '"./core.js"' has no exported member 'Result'.

1 import type { Result } from "./core.js";
                ~~~~~~

node_modules/@hyperjump/json-schema/lib/media-types.d.ts:7:21 - error TS2304: Cannot find name 'Response'.

7   parse: (response: Response, mediaTypeParameters: { [parameter: string]: string }) => Promise<[SchemaObject, string | undefined]>;
                      ~~~~~~~~

Found 2 errors in 2 files.

Errors  Files
     1  node_modules/@hyperjump/json-schema/lib/invalid-schema-error.d.ts:1
     1  node_modules/@hyperjump/json-schema/lib/media-types.d.ts:7

[jdesrosiers]

I renamed Result to OutputUnit and missed this one. I'm not sure why that error didn't come up when I run tests.

I didn't catch the issue with Response because it's included in recent versions of node. I'll update to use the Response type from node-fetch instead.

I especially appreciate feedback on types. Since I don't use TypeScript myself, it's easier for type issues to slip through.

[jdesrosiers]

I released v1.2.2 with the fix for Result->OutputUnit. I didn't fix Response other than to require node 18+. Let me know if that doesn't work for you and I'll see what I can do.

[harrisgilliam]

So.. I'll give it another test but I believe you will still need to import the Response type in media-types.d.ts to get the TS compiler to accept that definitions file. I don't think just being a default type in Node will be enough. But we'll see :-)

[harrisgilliam]

Yup. So... with TypeScript there are no "global types".. every type must be defined somewhere. So if you want to use a type that comes globally with Node you still need to import the type from Node to reference it. Similar to how I import the function readFileSync in my code example above.

> tsc

node_modules/@hyperjump/json-schema/lib/media-types.d.ts:7:21 - error TS2304: Cannot find name 'Response'.

7   parse: (response: Response, mediaTypeParameters: { [parameter: string]: string }) => Promise<[SchemaObject, string | undefined]>;

[jdesrosiers]

I don't generally use TypeScript myself, but I wrote my tests in TypeScript as a kind of smoke test to check my types. It's not perfect, but it's something. My point is, I use these types in my tests and they work fine. Apparently your setup is more strict than mine somehow? Can you share more about your setup (tsconfig, ts version, etc)? I'd like to emulate what you're doing so I can catch things like this sooner.

[harrisgilliam]

Ah.. yes...

TypeScript version 4.9.4

here is my tsconfig:

{
    "compilerOptions": {
        "declaration": true,
        "allowSyntheticDefaultImports": true,
        "allowJs": true,
        "checkJs": true,
        "forceConsistentCasingInFileNames": true,
        "lib": ["ES2020"],
        "module": "es2020",
        "moduleResolution": "nodenext"
        "noImplicitReturns": true,
        "noUnusedLocals": true,
        "noUnusedParameters": true,
        "noFallthroughCasesInSwitch": true,
        "outDir": "./dist",
        "sourceMap": true,
        "strict": true,
        "target": "ES2020"
    },
    "include": ["./src/"],
    "exclude": ["./node_modules"]
}

You may not have "strict" set to true... which relaxes the type checking rules. Since the only value TS adds is program correctness via type checking theres no reason to turn that off IMHO.

[jdesrosiers]

Looks like the problem is the lib option. By default, TypeScript includes a set of type definitions for built-in JS APIs based on the value of the target option. By declaring it the way you did, you're limiting the included types to strict ES2020 and excluding browser types including the fetch API which includes Response.

If you don't have a good reason to exclude browser types, the solution is to add "DOM" to the lib array or remove the lib option and get the default behavior.

I'm not sure what I'm going to do about this. One argument is that the library uses these types and if you want to use the library, you just need to configure TS properly to understand those types. The other argument is that TS makes this really confusing because fetch isn't just for browsers anymore and it doesn't make sense to lump it in with other browsers APIs anymore. It might be nice to use the node-fetch Response type for now just to help people avoid this gotcha.

[harrisgilliam]

I'm a big of configuring things narrowly until otherwise needed and having things out in the open. I think your suggestion of adding "dom" to the lib config is reasonable... although I'd rather have a type definition explicitly declared instead of relying upon the compiler to silent add it behind the scenes. Also your point about fetch not really belonging the the "dom" lib definition anymore. When the fetch built into Node is no longer experimental then you can just import that explicitly. Until then I think using node-fetch is a good compromise.

Sorry for the delayed response... by company just announced its closing this week so I've been a little distracted :-(

[jdesrosiers]

When the fetch built into Node is no longer experimental then you can just import that explicitly.

Are you sure that the reason it's not included is because it's experimental? Especially because it's enabled-by-default, I would expect that it would be included if they intended for it to be included. I think it's likely that it was a conscious choice to include it in only one place and that place was "DOM".

In any case, I think I'm going to add the node-fetch type, but I need to think through some things first. Mostly I want to make sure it's 100% compatible with the built-in type. Otherwise, I can't migrate to the built-in type later without a breaking change.

Good luck with the other thing. I'm sure you'll land on your feet, but it sucks to have go through that unplanned.

[jdesrosiers]

I went with including the types, but I also switched from node-fetch to undici which is the implementation nodejs uses, so compatibility issues should be less likely in the future.