Fix type imports

[remcohaszing]

ESM imports require a file extension. Also directory imports don’t resolve to index.js. This goes for source code as well as type definitions.

Modern module resolution settings validate this. The correct "moduleResolution" value for libraries that target Node.js 16 or lower is "Node16". For libraries that target Node.js 18 or greater should set it to "Node18" or "NodeNext". For code that gets bundled, the correct option is "Preserve". All other options are outdated.

The config in @ljarb/tsconfig/node sets this correctly, but it contains a syntax error.

[ljharb]

this is using require, so it's not an ESM import.

[remcohaszing]

Your reasoning makes sense, yet I see a type error.

node_modules/async-function/index.d.mts:1:35 - error TS7016: Could not find a declaration file for module '.'. 'node_modules/async-function/legacy.js' implicitly has an 'any' type.

1 import getAsyncFunction = require('.');
                                    ~~~


Found 1 error.

[ljharb]

Suggested change

	import getAsyncFunction = require('./index.js');
	import getAsyncFunction from './index.mjs';

this should also work, but fwiw this seems like a typescript bug

[remcohaszing]

Your suggestion contains a circular type definition.

The following works, but only if allowSyntheticDefaultImports is enabled:

import getAsyncFunction from './index.js';

I agree this seems like a TypeScript bug. But still, this PR as-is solves the problem, also for current TypeScript versions where the bug is present.

[ljharb]

TS's module system is broken if allowSyntheticDefaultImports isn't enabled, so I'm not too worried about that - everyone should always have that enabled, full stop.

With either change, though, i'd love some way to ensure it doesn't regress. Why didn't TS catch this locally?

[remcohaszing]

TS's module system is broken if allowSyntheticDefaultImports isn't enabled

This is only true for CJS / ESM interop.

With either change, though, i'd love some way to ensure it doesn't regress. Why didn't TS catch this locally?

You’re using "moduleResolution": "classic". You should never use this. Always set it to "Node16" (if targeting Node.js < 18) / "Node18" (if targeting Node.js >= 18 / "NodeNext" (if you’re less concerned about future updates) for libraries.

[ljharb]

but most of the package is a CJS library, which should be using classic resolution, no?

[remcohaszing]

No, that’s a common misconception. The Node16 (and greater) module resolution handles both CJS, ESM, and dual publishing.

Now that I’m thinking about it, you could test against multiple module resolutions using multiple tsconfigs and TypeScript project references. This would allow you to test against the Node10 module resolution too. I don’t think I’ve seen people do this before, but you could do this.

As for the Classic module resolution: I don’t know its details too much. This was already outdated when I started using TypeScript.

[ljharb]

hmm, according to the comments left by tsc --init, i'm using node10, not classic.

[remcohaszing]

According to tsc --showConfig you left it unset. According to the documentation this means moduleResolution defaults to Node10 for module CommonJS. So you’re right, I was mistaken. :)