esModuleInterop: true cause runtime error

[themez]

Hi there, problems happened when I toggled esModuleInterop flag for my project, I have to because one library it depends enabled this flag.

TypeScript Version: 2.7+

Search Terms:
esModuleInterop
allowSyntheticDefaultImports

Code

import * as apmStar from 'elastic-apm-node'
import SentryDefault from '@sentry/node'

apmStar.start // undefined
SentryDefault // undefined

Explains:

• for @sentry/node it is an es module, so esModuleInterop has no effects on it, but with allowSyntheticDefaultImports you can import its default, which property it does not export, so you got an undefined error at runtime
• for elastic-apm-node, it is a commonjs module, and it exports an instance, start is a prototype method of it, so it is lost after __importStar.

But these problems are not informed in the document, guess we could discourage enabling it for library projects?
And I wonder if we could improve type checking for these situations, for example:

• for apmStar, it is imported as esmodule namespace, so an type error could be thrown when accessing .start method on it.
• for SentryDefault, it is a esmodule, allowSyntheticDefaultImports could be disabled for it

Expected behavior:
Error emitted at compiling time
Actual behavior:
Compiled successfully, but got an error at runtime.

Playground Link:
https://repl.it/@themez1/esModuleInteropTest

Related Issues:
#28009
#33954
#36026

[themez]

Update: I found in [checker.ts], there is a checking on __esModule export, if exists, hasSyntheticDefault would be set to false.
However, this checking does not work cause __esModule is not shown in d.ts file(maybe it should), only exists in .js file.

Am I digging into the right direction?

(https://github.com/microsoft/TypeScript/blob/master/src/compiler/checker.ts)

const hasSyntheticDefault = canHaveSyntheticDefault(file, moduleSymbol, dontResolveAlias);

//function canHaveSyntheticDefault
...

// It _might_ still be incorrect to assume there is no __esModule marker on the import at runtime, even if there is no `default` member
// So we check a bit more,
if (resolveExportByName(moduleSymbol, escapeLeadingUnderscores("__esModule"), /*sourceNode*/ undefined, dontResolveAlias)) {
    // If there is an `__esModule` specified in the declaration (meaning someone explicitly added it or wrote it in their code),
    // it definitely is a module and does not have a synthetic default
    return false;
}

[RyanCavanaugh]

These flags are meant for you to set depending on how your module loader behaves -- if your loader doesn't generate synthetic defaults for you, then it's not correct for you to set allowSyntheticDefaultImports. Same for esModuleInterop; if your loader doesn't have that behavior, then you can get yourself into trouble as shown.

Basically this is just a configuration error in your program; you should be setting the flags such that things that don't work at runtime become compile-time errors. It's not meant to create an arbitrary matrix of behavior in any loader environment.

[themez]

@RyanCavanaugh Thanks for your reply!
To my understanding, synthetic defaults are generated by esModuleInterop. It adds __importDefault to your require(). but it only generates for non-esmodule:

var __importDefault = (this && this.__importDefault) || function (mod) {
    return (mod && mod.__esModule) ? mod : { "default": mod };
};

allowSyntheticDefaultImports default value follows esModuleInterop. If you disable allowSyntheticDefaultImports explicitly with esModuleInterop enabled, that would cause problems.

For example, elastic-apm-node is a commonjs module, which's exports is a class instance, disabling allowSyntheticDefaultImports make you cannot import it's default (which is generated by esModuleInterop, and of course this module's d.ts doesn't included it).
The problem is that if you don't import this module's default, you have to import star of this module, which is originally a class instance, but turned into a plain object by esModuleInterop:

var __importStar = (this && this.__importStar) || function (mod) {
    if (mod && mod.__esModule) return mod;
    var result = {};
    if (mod != null) for (var k in mod) if (k !== "default" && Object.prototype.hasOwnProperty.call(mod, k)) __createBinding(result, mod, k);
    __setModuleDefault(result, mod);
    return result;
};

The result is you cannot access any prototype method of it.

// tsconfig:
// "esModuleInterop": true,
// "allowSyntheticDefaultImports": false,
import * as apmStar from 'elastic-apm-node'
import apmDefault from 'elastic-apm-node' // compiling error

console.log(apmStar.start) // runtime error, apmStar is a plain object, has no prototype methods
console.log(apmDefault.start)

https://repl.it/@themez1/esModuleInteropTest-1#index.ts

---

I think the problem is that currently esModuleInterop and allowSyntheticDefaultImports are conflicting.
The purpose of introducing esModuleInterop is to make the import statement align to ES spec: import star symbol can only be a plain object, not a class or instance or anything else. So it should have two more principles to follow(which it currently doesn't):

• Do not affect compiling behavior of module which IS an esmodule
• Which esModuleInterop enabled, a compiling error should be thrown when your importing violate ES spec. For example when you import a non-plain-object module using import star style.

That's what I got so far, if there's anything I misunderstood, please kindly correct me.

[RyanCavanaugh]

Concisely, what do you think TS should do differently and why?

[themez]

When esModuleInterop flag is true:

1. For es module which does not have default export，emit compiling error for importing default on it
2. For commonjs module which export non-plain object such as class, emit compiling error for importing star on it

The reason is if not do so, there would be runtime errors.

As #41916 shown, I think the checker is intend to do so for 1, however the implementation is not completed.

BTW, I'm glad to help on this issue only if you agree with this approach. (Might need some help to be familiar with checker internals)

[octogonz]

We encountered a similar issue in microsoft/rushstack#2526 . The repro is very easy:

1. Create a TypeScript project that imports the colors package (version 1.2.5 in our case)
2. Enable esModuleInterop=true in tsconfig.json
3. Observe that this is the correct way to import the library:

   // Emitted as:  const colors_1 = __importDefault(require("colors"));
   import colors from 'colors'; 
   
   console.log('Some ' + colors.red('colored text')); // works great

4. Now try this way:

   // Emitted as:   const colors = __importStar(require("colors"));
   import * as colors from "colors";  // compiles without any error message 
   
   // Fails at runtime with "TypeError: colors.red is not a function"
   console.log('Some ' + colors.red('colored text'));

@RyanCavanaugh in our case, setting allowSyntheticDefaultImports=false reports an error for the correct import form. So that doesn't seem to be a solution.

Setting esModuleInterop=true causes the compiler to silently accept code that will fail at runtime. These bugs may lurk in the code base for a long time until someone hits that code path (and bothers to report it). Maybe something is wrong with the typings for the colors package, but we've also encountered this with various other popular Node.js packages.

What's the right way to prevent these mistakes? We never had these problems with esModuleInterop=false.

[dmichon-msft]

When writing JavaScript for a commonJS it is natural to do:

const colors = require('colors');
// or
const { red } = require('colors');

These intuitively map to:

import * as colors from 'colors'; // module.exports
// or
import { red } from 'colors'; // module.exports.red

Which is (approximately) what we get when setting esModuleInterop = false. Live bindings mean that we pay the runtime cost of the property access at every call site instead of just the import line, but the shape nonetheless remains the same. The above import statements work reliably and consistently for all CommonJS modules.

Enabling esModuleInterop = true instead makes the behavior vary at runtime based on whether or not module.exports.__esModule === true for the imported module; if it does, then the original pattern continues to work, but if it does not, then suddenly we have const colors = { default: require('colors') } (inlined) which is not something a developer would naturally write.

The main problem I see with the flag is that it makes the runtime behavior cease to be statically determinate at compile time, which at the very least should come with some significant warnings in the documentation. For projects that will only ever be executed as CommonJS, "the module exotic namespace object is not a function" is a job for the typings of the particular module, not the compiler in general.

I think the only real reason to enable this flag is if you are transpiling the same project to both ESM and CommonJS, will be natively executing both versions in NodeJS (instead of one or the other, for some reason), and therefore need to ensure that the behavior is consistent between the two. Since top-level await was introduced and cannot be transpiled to valid CommonJS, it does seem like a bit of a niche scenario.

[themez]

@dmichon-msft I think the reason to enable this flag is to eliminate importing which does not conform to ESM spec.
For example, a CommonJS module which export a class is not a valid ESM:

module.exports = class A {}

without esModuleInterop flag enabled, you have to import it like this:

import * as A from 'a'

This does not conform to ESM spec, because an ESM must be a plain object.

esModuleInterop turn the dependent module a valid es module, then you can write:

import A from 'a'

There are so many existing CommonJS library, we have to do so to using them in our ESM projects.

[ExE-Boss]

Note that if you’re using native NodeJS ES modules, then you should set esModuleInterop: false and allowSyntheticDefaultImports: true in your tsconfig.json, then:

import A from 'a';

will work at runtime.

---

Refs: https://nodejs.org/api/esm.html#esm_commonjs_namespaces

[khuttun]

for elastic-apm-node, it is a commonjs module, and it exports an instance, start is a prototype method of it, so it is lost after __importStar.

FYI, this behavior has its own bug reported here: #29687

The main problem I see with the flag is that it makes the runtime behavior cease to be statically determinate at compile time, which at the very least should come with some significant warnings in the documentation.

The docs now warn about the treatment of own vs inherited properties: https://www.typescriptlang.org/tsconfig#esModuleInterop

[walkerburgin]

I noticed that with esModuleInterop enabled, compilation fails when you attempt to default import from another TypeScript file that has no default export:

src/importFailsAsExpected.ts:3:8 - error TS1192: Module '"packages/foo-app/src/foobar"' has no default export.

3 import foobar from "./foobar";
         ~~~~~~

Found 1 error.

But if you take the same file and move it in to another package, you no longer get an error:

import foobar from "@test/foo-lib";

That surprised me! Interestingly, if you add export const __esModule: true; to the other package's .d.ts file (prompted by #41916) then it errors the way I was anticipating. I put together a small repro here if it's helpful.

If it's possible to somehow extend the type checking to handle both of these cases consistently (either by emitting the marker into .d.ts files or something else), I think it would go a long way towards helping.

[frank-dspeed]

@weswigham can you please read the above comment and maybe tell me what you think about the __esModule true marker in the declarations in general? do you see any breaking scenarios with that i ask because i think about adding that to rollup when creating d.ts files.

[weswigham]

It's definitely a guaranteed indicator that the file is a cjs-format module that's masquerading as an es module and has no default presented by ts/babel's import helpers.

You shouldn't put it in the declarations for actually-esm format modules, however, since those don't use any import helpers :P