fix(36909): wrong error message when trying to named-import an export

[a-tarasyuk]

Fixes #36909

[andrewbranch]

Thanks @a-tarasyuk, that was fast! Code looks good but I want to get @DanielRosenwasser to double check my proposed messages and the conditions for choosing between them. It’s also worth looking at where the similar TS2497 is issued:

This module can only be referenced with ECMAScript imports/exports by turning on the '{0}' flag and referencing its default export.

I noticed that it does some logic to determine whether to report the flag allowSyntheticDefaultImports or esModuleInterop, but I haven’t internalized why. It also may be reasonable to try to come up with a message that doesn’t duplicate that part of TS2497, as I think they’ll always appear together.

[a-tarasyuk]

@andrewbranch Sure, I opened PR to start the discussion.

[andrewbranch]

@a-tarasyuk can you take this out of draft state?

[DanielRosenwasser]

My thinking is to always suggest esModuleInterop because

• it disables namespace imports from becoming callable/constructable
• it will work for other module formats too

Thoughts @weswigham?

One thing that I'm not totally sure about whether this error message is "fine" for plain objects and namespaces - we're effectively nudging people to import it as a default, but arguably

import * as x from "./x`;

is reasonable for most TypeScript users importing from

// x.d.ts
declare let x: { foo(): x.Foo }
namespace x {
  export interface Foo {
    blah: number;
  }
}
export = x;

[andrewbranch]

TS9423: 'Foo' can be imported literally any way except the way you have it now.

[DanielRosenwasser]

I need a "laughing but crying" reaction on GitHub 😅😂😭

[a-tarasyuk]

@DanielRosenwasser

My thinking is to always suggest esModuleInterop because

Do you mean that just need to check if the option esModuleInterop is negative then show the message with the suggestion to enable it and avoid all other suggestions?

[sandersn]

@DanielRosenwasser does this need to go into 3.9? If so, can you reply to @a-tarasyuk's question?

[a-tarasyuk]

For the upcoming release, we can use the message - Module "./a" has no exported member 'Foo' (pre-3.8 message). For suggested changes, we can make a new feature and continue the discussion there. What do you @DanielRosenwasser, @andrewbranch think?

[DanielRosenwasser]

This can go into 3.9.1 (RC).

@a-tarasyuk I just meant that instead of allowSyntheticDefaultImports we might be better off suggesting esModuleInterop in these new error messages instead - though I might be missing something.

[andrewbranch]

My suggestion was to use the same logic as here to populate the option name (getTargetOfImportClause, currently line 2365 of checker.ts):

But, I’m not totally sure the module target is actually a good heuristic, given that Webpack and other bundlers are going to do their own thing with modules—in other words, saying your module target is es2015+ doesn’t necessarily mean you’re ultimately going to run with es2015 modules at the end of your pipeline, and you may still need interop. Personally, I’ve never been in a situation where allowSyntheticDefaultImports was correct.

[a-tarasyuk]

Thanks for the detailed feedback :). Maybe we can simplify messages something like this

if there is exportedEqualsSymbol
    check option = moduleKind >= ModuleKind.ES2015 ? "allowSyntheticDefaultImports" : "esModuleInterop"
    show message 
        error = {value} can only be imported by using a default import
        add related info to error
            This module is declared with using export= and can only be used with the {option} flag
otherwise 
   show message `Module has no exported member` 

What do you @DanielRosenwasser, @andrewbranch think?

[andrewbranch]

{value} can only be imported by using a default import

This module is declared with using export= and can only be used with the {option} flag

If the module kind is < es2015, these statements aren’t true 😄 because it can be imported using an ImportEqualsDeclaration.

Also, I only just now noticed the existing message I screenshot (emphasis mine):

This module is declared with using 'export =', and can only be used with a default import when using the 'esModuleInterop' flag.

Declared with using?! Either “with” or ”using” needs to be deleted. What a mess.

@a-tarasyuk I think the best approach is just to keep what you have, but replace allowSyntheticDefaultImports with esModuleInterop unconditionally in the diagnostic messages. Does that sound good to everyone?

[andrewbranch]

Thanks for hanging in there; this looks good to me!

[DanielRosenwasser]

I think the question is still whether we should ever recommend allowSyntheticDefaultImports in the module >= es2015 case - the benefit of esModuleInterop is that it disallows code like

// foo.d.ts
declare namespace foo {}
declare function foo(): void;

export = foo;

// bar.ts
import * as foo from "foo";

foo(); // error in esModuleInterop, not in allowSyntheticDefaultImports

Is that accurate @weswigham?

[weswigham]

Yeah, that's the difference for that target.

[andrewbranch]

Sure, but none of these error messages recommend a namespace import. They recommend esModuleInterop + a default import. The benefit of esModuleInterop is you can use a default import on a CommonJS export and actually expect it to work at runtime.

I’m honestly confused about what allowSyntheticDefaultImports is for, because I’ve never used a runtime where it works. I just searched for it in release notes, and it was added in 1.8 for interop with SystemJS, which is a term that I probably read on a blog post in 2014, and otherwise have only seen on our tsconfig reference page.

So yeah, I don’t really see any reason to recommend allowSyntheticDefaultImports.

[DanielRosenwasser]

I’m honestly confused about what allowSyntheticDefaultImports is for

It was to recognize the behavior of Webpack (and eventually Babel once it could understand TS code) during type-checking. We didn't want to change our own emit, and eventually we recognized that it would be useful.

esModuleInterop:

• provides that specific behavior when you're targeting commonjs
• basically acts as allowSyntheticDefaultImports when your module is es2015+
• yells at you for calling/constructing namespace imports because you have an alternative that will work in every module target

It's just a better newer version of the flag.