Ensure moduleType is structured during cloneTypeAsModuleType

[jakebailey]

Fixes #51099

[weswigham]

We should probably just getApparentType on the module type, to resolve primitives to the structured object type backing them - that way you can pull out toString if you really wanted to for some reason.

[jakebailey]

That does change:

import * as exportSymbol from "./exportSymbol.cjs";
->exportSymbol : symbol
+>exportSymbol : { toString(): string; valueOf(): symbol; readonly description: string | undefined; [Symbol.toPrimitive](hint: string): symbol; readonly [Symbol.toStringTag]: string; }

But it unfortunately still makes any and unknown (maybe others I'm not thinking of?) crash unless I keep the structured if/else.

[weswigham]

Hm. Yeah, makes sense I guess, since any and unknown are their own apparent type (while symbol and the like have global interface types). Seeing the ns in import * as ns be of type primitive string directly feels wrong given how namespace member hoisting works, while "an object with all the members of a string primitive" seems more allowable. Also, honestly, we're doing this to strip signature-y-ness, stripping primitive-y-ness at the same time makes sense. How about something like const resolvedModuleType = getApparentType(moduleType).flags & StructuredType ? resolveStructuredTypeMembers(getApparentType(moduleType) as StructuredType) : emptyObjectType;? export = any case aside, I think that'd definitely do what we'd want, and I'm not sure I like any other way we could handle the export = any case.

[jakebailey]

In #51099 the goal is to actually make the import any, so this would make it the empty object which seems restrictive. The issue reports that the way this came up is that they switched to d.cts and the bug appeared.

The weird thing is that this code already will return any of those non-structured types, it's just that in the special condition of default and isESMFormatImportImportingCommonjsFormatFile(...) === true, it always tries to remove signatures, which fails for non-structured types. So it seems like if you don't have any call signatures and you set up your imports just right, you can observe these values anyway because the code I'm touching is never actually run.

[andrewbranch]

In #51099 the goal is to actually make the import any

If that’s the goal, I don’t think it’s a goal we should support. Whatever is the target of module.exports (or export = in TS syntax) will be the default module record when you import * that module:

So I think it stands to reason if you export = something that’s any, if you import * it you should get something like { default: any }

[andrewbranch]

Aside from the crash, our checking for this appears to be super wrong right now.

[jakebailey]

Yeah. I'm not super familiar, but it seemed suspect. I just don't know quite what the intended behavior is here.

I could reasonably remove the debug check and allow this code to proceed as it did before, if the answer isn't obvious.

Maybe we need the real example to provide an alternative? @dsherret

[dsherret]

It seems doing var x: any; export = x is sometimes used to make a module have an any type (https://stackoverflow.com/a/31729755/188246 -- though, in this scenario since they're using an ambient module, they could do declare module 'Foo';... answer is old, but it's the top SO result on google for "any module typescript").

It's been used for some time in Deno when the types of a module can't be resolved in certain cases in order to have an any type, which can be a nice experience for our users to just not worry about typescript in that case. The problem though, is now with our upcomming support for npm packages, we get these errors when resolving to deno:///missing_dependency.d.ts as a .d.ts file, which has the text declare const __: any; export = __;:

error: TS1259 [ERROR]: Module '"deno:///missing_dependency.d.ts"' can only be default-imported using the 'allowSyntheticDefaultImports' flag
import esmDefault from "npm:@denotest/esm-import-cjs-default"; // note: this is not a real npm package
       ~~~~~~~~~~
    at file:///V:/deno/cli/tests/testdata/npm/esm_import_cjs_default/main.ts:6:8

TS2594 [ERROR]:     This module is declared with 'export =', and can only be used with a default import when using the 'allowSyntheticDefaultImports' flag.
    export = __;
    ~~~~~~~~~~~~
        at deno:///missing_dependency.d.ts:2:1

Resolving that to instead a .d.cts file makes those diagnostics go away, but then surfaces that debug assert. It would be nice if when we can't resolve the types of an npm package (or some other module) we could tell it to resolve to an any module type. Perhaps getting rid of those diagnostics would solve it (maybe I could just not surface them)?

Edit: For now what I've done is ignored these diagnostics. Edit 2: No, this was not sufficient. The real issue/fix was #51321

So I think it stands to reason if you export = something that’s any, if you import * it you should get something like { default: any }

That makes sense to me. Edit: actually, maybe not. See two comments below.

[jakebailey]

It's been used for some time in Deno when the types of a module can't be resolved in certain cases in order to have an any type,

Edit: For now what I've done is ignored these diagnostics.

Out of curiosity, is it possible to instead ignore the "missing types" error on the module, rather than hiding the errors in the library itself?

[dsherret]

So I think it stands to reason if you export = something that’s any, if you import * it you should get something like { default: any }

@andrewbranch actually, I think this should be the any type because if the any is an object then then there could be named exports?

[andrewbranch]

I mean, sure, you can’t prove that any for the whole module type is wrong, but that value still conforms to { default: any }. It represents the most specific type we can infer about the module if all we know is there is a module.exports of type any.

[dsherret]

@andrewbranch it could be defined that way, but I think it would be nice if an any type were used that it could be anything since using a namespace import property or named import might be valid at runtime. To me, it seems to be more in line with the spirit of any for that to not be restrictive.

I also can't think of another way that typescript could be used to allow anything to be used on an import declaration, or is there?

[andrewbranch]

I guess you’d have to assign or cast to any or a record of any. Or, of course, access them off .default

[dsherret]

I mean, without affecting the importing code (which would solve our case in Deno where we don't have control over the user's code) (edit: nevermind, see next comment). For another example, someone might be migrating some mixed esm/cjs code to TypeScript and as part of that, they want to mark a cjs module as any and they have many modules that import it using namespace and named imports. They might want to keep most of the code as-is, but I don't believe there would be a way to do this unless an any type exported from the cjs module allowed for anything to be imported from it.

Anyway, it seems inconsistent for any to be a constrained set here, but I'm not too familar with all the edge cases of everything. In my opinion, it being any aligns with how module.export = ... works where different values have different impacts on the imports and aren't constrained to just { default: any }. I think the any type is useful at opting out of TypeScript's type checking and I think it could be useful here for allowing anything to be imported.

[dsherret]

Actually, I've discovered that #51321 is our main issue in Deno (related to the diagnostics I was getting earlier, but didn't investigate until now), so I don't really mind whatever is decided here for ModuleKind.ESM importing ModuleKind.CJS.

[jakebailey]

I've revived this PR with the approach suggested in #51136 (comment), which makes the type match what we get at runtime; see e7aed81 (#51136).