allowSyntheticDefaultImports not respected when using `ModuleKind.ESNext`

[dsherret]

Bug Report

Currently, allowSyntheticDefaultImports is not respected when a file is ModuleKind.ESNext.

A lot of people in the ecosystem incorrectly define ES modules using export = and so this causes some issues.

The root cause is this usageMode === ModuleKind.ESNext in checker.ts. Removing that expression fixes the issue:

function canHaveSyntheticDefault(file: SourceFile | undefined, moduleSymbol: Symbol, dontResolveAlias: boolean, usage: Expression) {
    const usageMode = file && getUsageModeForExpression(usage);
    if (file && usageMode !== undefined) {
        const result = isESMFormatImportImportingCommonjsFormatFile(usageMode, file.impliedNodeFormat);
        if (usageMode === ModuleKind.ESNext || result) {
            return result;
        }
        // fallthrough on cjs usages so we imply defaults for interop'd imports, too
    }

🔎 Search Terms

allowSyntheticDefaultImports ModuleKind.ESNext

🕗 Version & Regression Information

• 4.9.0-dev.20221026
• main

🙁 Actual behavior

Errors like the following, even though allowSyntheticDefaultImports is true:

Module '"https://esm.sh/v96/@types/sanitize-html@2.6.2/index.d.ts"' can only be default-imported using the 'allowSyntheticDefaultImports' flag

🙂 Expected behavior

No type errors, but maybe it could be argued the error message would improve to not mention the allowSyntheticDefaultImports flag. I would argue for no type errors because of the prevalance of people incorrectly using export = to describe ES modules.

[RyanCavanaugh]

@weswigham / @andrewbranch thoughts?

[andrewbranch]

I can’t reproduce this. Can you provide a repro with tsconfig and the importing file?

Bit of a tangent:

A lot of people in the ecosystem incorrectly define ES modules using export = and so this causes some issues.

This could be true but from my experience on DT I usually see the opposite (people wanting to write export default for a module.exports =). It’s worth noting that sanitize-html is a CJS module. The typings for it are correct. But esm.sh is going to magically convert the implementation to an ES module with export default, while doing nothing to the contents of the corresponding @types package. So while the error message you’re getting definitely looks buggy, I think esm.sh’s transforms are going to present a big issue for type declarations in general.

[dsherret]

Sorry, I should have done that at the start: https://github.com/dsherret/tsc-issue-51321

Essentially...

// other.mjs
export default function example() {
  return 5;
}

// other.d.mts
declare function example(): 5;
export = example;

// main.mts
import example from "./other.mjs";
example();

// tsconfig.json
{
  "compilerOptions": {
    "target": "ESNext",
    "module": "NodeNext",
    "allowSyntheticDefaultImports": true,
    "strict": true
  }
}

> npx tsc
main.mts:1:8 - error TS1259: Module '"V:/tsc-issue-51321/other"' can only be default-imported using the 'allowSyntheticDefaultImports' flag

1 import example from "./other.mjs";
         ~~~~~~~

This could be true but from my experience on DT I usually see the opposite (people wanting to write export default for a module.exports =).

Yeah, I ran into one of these today.

It’s worth noting that sanitize-html is a CJS module. The typings for it are correct. But esm.sh is going to magically convert the implementation to an ES module with export default, while doing nothing to the contents of the corresponding @types package. So while the error message you’re getting definitely looks buggy, I think esm.sh’s transforms are going to present a big issue for type declarations in general.

Yup, the typings are correct for a cjs module, but because esm.sh transforms the javascript to an es module without also changing the type declarations it makes it become incorrect ☹. Apparently allowSyntheticDefaultImports was enabled in the past in Deno to allow this exact scenario, so disabling it now would be quiet breaking.

[andrewbranch]

Oh, well the key to reproducing it is that the declaration file that uses export = is itself an ES module, which I honestly have no idea how to reason about. export = in an ESM file is an error, but it’s suppressed in declaration files. It seems totally nonsensical to me and maybe the error shouldn’t be suppressed. I’ll have to see if @weswigham has a mental model of what this means.

[dsherret]

Oh, well the key to reproducing it is that the declaration file that uses export = is itself an ES module, which I honestly have no idea how to reason about. export = in an ESM file is an error, but it’s suppressed in declaration files.

Yup, exactly. If you change everything to esm and switch to "module": "ESNext" it will type check just fine which is why we're now encountering this one in Deno while adding Node CJS import support (because people have been describing default exports in es modules with export = in a declaration file).

[weswigham]

Hm, so I think I want to do 2 things here.

1. Unsuppress the error on export = in an esm mode declaration file. That should make it obvious where something nonsensical is taking place.
2. Adjust the now-follow-on error message to, rather than mentioning allowSyntheticDefaultImports (which is already on), to instead simply complain that the default is missing (which is less inaccurate).

[dsherret]

That makes sense, but I think it's going to cause a lot of pain in Deno given how much of the Deno ecosystem seems to rely on this because it never errored before. Probably worth it in the long run because people shouldn't have been doing that anyway.