Improve import type support for commonjs exports

[sandersn]

This PR makes getTypeFromImportTypeNode a little more like getExternalModuleMember: for JS files, it now uses both getTypeOfSymbol and getExportsOfSymbol, and uses whichever one returns a symbol. This allows using arbitrary properties of a CJS export= as types in JSDoc; previously a special case in the binder enabled only CJS export= where all properties were shorthand assignments.

Fixes #49195

[andrewbranch]

So, this is different from what you’d get if mod.js had said module.exports.Thing = Thing, right? I guess I was under the impression that we intended to bind the object literal property assignment similarly.

[sandersn]

Yes, that's a good point. IIRC this is an existing limitation of the code, but I'll take another look to see how to fix it.

[sandersn]

So: currently the code has special lookup for object literal property assignments, but it doesn't try to follow value-only symbols that come from the properties as if they were aliases, which is what is needed to make this work. Either that, or the binder needs to bind the object literals so that the property assignments have symbols that are marked as aliases.

[sandersn]

There is existing alias lookup code for property assignments, so it might not be too difficult to add this feature. This isn't a high-priority thing so I may try to add it and see where it goes.

[andrewbranch]

Similarly, this sure looks like it would be referring to the constructor type of Thing, but it’s the instance type here.

[sandersn]

No, it's the constructor type. See the types baslines at https://github.com/microsoft/TypeScript/pull/49745/files#diff-88e8eb66de54881e4ab7d48ec9f25031a7327dc04e169d0f32347b2b49d22068R133-R134

Also it's in a TS file so the new JS rule doesn't apply.

[andrewbranch]

What happens when you do typeof in the JS file? Is it possible to get the constructor type?

[sandersn]

Yes. I added a test for that.

[sandersn]

The new commit improves the binder for property assignments with class expressions and identifiers. Read the commit text for full details.

[sandersn]

Currently the breaking change is that, previously, a conflicting typedef and class would export the value type of the class and the type type of the typedef. Now both the value and type type of the class are exported. I think this makes sense because the class is explicitly exported; but there should be an error issued too.

Before:

/** @typedef {number} C */
module.exports = {
  C: class { }
}

would previously import as:

import { C } from './mod'
var c: C = 12
c = new C() // error, 'C' not assignable to 'number'

and now imports as

import { C } from './mod'
var c: C = 12 // error 'number' not assignable to 'C'
c = new C()

[sandersn]

Comments from yesterday's discussion

[sandersn]

maybe should just bind everything and let the checker resolve straight to the property assignment's initialiser

[sandersn]

Later: I tried this (except for binding non-aliases as properties) and decided that to do this correctly, export= would need to change to work the same way.

[sandersn]

I'm not sure what to do about this, but I think I managed to convince myself that it is equivalent to this typescript, which also fails to include the typed meaning of classes, except in import types:

// @file: mod.ts
class Thing { x = 1 }
export = { Thing }
// @file: index.ts
import { Thing } from './mod'
declare const thing: Thing

This gives two errors:

1. Can't use ES imports on './mod' (please use esModuleInterop + default import)
2. 'Thing' is a value, not a type. Did you mean 'typeof Thing' ?

(2) is the same problem as we encountered in JS for module.exports =. (2) doesn't happen with getTypeFromImportTypeNode in TS because it uses the getPropertyOfType fallback for typeof.

Treating module.exports = { A, B, C } the same as export { A, B, C } is tempting but ultimately misleading, because it's closer to export = { A, B, C }...which has the limitation I just described.

I tried taking the esmoduleInterop advice:

// @esmoduleinterop: true
// @file: mod.ts
class Thing { x = 1 }
export = { Thing }
// @file: index.ts
import mod from './mod'
declare const thing: mod.Thing

But this tells me (correctly?) that mod isn't a namespace. It's an object, so in typescript the compiler refuses to look for types inside it, knowing it must contain only values.

[sandersn]

The bottom line is: module.exports= and export= should behave the same, which today they largely do.

I got pretty close to making module.exports= behave the same as export { ... } + property-fallback in the binder, so it might be worthwhile to put that in a follow-up PR with code to make export= behave the same way. However, I think for this fix I should revert the binder changes and keep the original checker workaround.