Runtime error with *esModuleInterop* when CJS module has an export named "default"

[marcelltoth]

TypeScript Version: 3.9.2 & 4.0.0-dev.20200512

Search Terms: __setModuleDefault, __importStar, redefine, ts-lib

Code

Check out the full example at https://github.com/marcelltoth/typescript-bug

File dangerous-module.js (commonjs module)

const a = {
    someConstant: 2
};
module.exports = a;
// Allow use of default import syntax in TypeScript
module.exports.default = a;

This is a pattern seen in real world, when the authors of CJS modules are trying to be nice with us TypeScript users. This way the module is nicely consumable from TS without esModuleInterop. The pattern is used by - for example - axios

I have esModuleInterop and therefore allowSyntheticDefaultImports turned on. Then in another file:

File index.ts

import m, {someConstant} from './dangerous-module.js';

console.log(m);
console.log(someConstant);

I build it via tsc, then run node dist/index.js on the output.

Expected behavior:

The code logs to the console.

Actual behavior:

It throws on the import line like so:

/home/marcelltoth/source/typescript-bug/dist/index.js:10
Object.defineProperty(o, "default", { enumerable: true, value: v });
^

TypeError: Cannot redefine property: default
at Function.defineProperty ()
at /home/marcelltoth/source/typescript-bug/dist/index.js:10:12
at __importStar (/home/marcelltoth/source/typescript-bug/dist/index.js:18:5)
at Object. (/home/marcelltoth/source/typescript-bug/dist/index.js:22:29)
at Module._compile (internal/modules/cjs/loader.js:1158:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:1178:10)
at Module.load (internal/modules/cjs/loader.js:1002:32)
at Function.Module._load (internal/modules/cjs/loader.js:901:14)
at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:74:12)
at internal/main/run_main_module.js:18:47

Playground Link: https://github.com/marcelltoth/typescript-bug

Related Issues: #37113

[marcelltoth]

Findings

I have already spent quite a lot of time debugging the issue, here's what I found.

The issue only arises when the __importStar helper is used. One way to make that happen is to have an import that mixes default and named imports: utilities.ts#L44

The actual problem is inside the __setModuleDefault helper that __importStar calls.

That one is defined as:

var __setModuleDefault = (this && this.__setModuleDefault) || (Object.create ? (function(o, v) {
    Object.defineProperty(o, "default", { enumerable: true, value: v });
}) : function(o, v) {
    o["default"] = v;
});

Now if the module already has an export called default, it will try to overwrite such module.exports.default, which fails because said property is not configurable.

Proposal

In my view, __importStar / __setModuleDefault should only try to write module.exports.default if said property does not exist.

While this is somewhat of a compromise:

1. this behavior would be much preferable to outright crashing.
2. I think we can safely assume that the reason for any modern CJS module to have a default export is that they expect us to use it as a default import. (Just like what happens without esModuleInterop.

I'll be happy to submit a PR as soon as this is accepted. Working branch: https://github.com/marcelltoth/TypeScript/tree/fix/safe-set-module-default

[harmon]

Same issue here using Typescript v3.9.2 with Sequelize v5.21.8, which uses a "default" on their exports, which is causing TS to choke on: https://github.com/sequelize/sequelize/blob/master/lib/sequelize.js#L1356

Reverting back to v3.8.3 fixes this for us as an interim solution.

[devshorts]

This is unfortunate since the promise.all fixes in 3.9 we have been waiting on for a long time now. Are there any suggested workarounds (short of downgrading) that would allow us to move to 3.9 without getting bit by this (we are hitting it with axios and some other libraries as well)

[jrnelson90]

I can confirm having this same issue using TS 3.9.2, so I'll also be reverting all my work to v3.8.3 till this closes

[marcelltoth]

@devshorts What I ended up with is to split my imports in a way so the compiler emits an __importDefault instead of the __importStar.

Axios for example does not export any constant but the default, all the rest are just types so you can do:

// instead of 
import axios, { SomeAxiosType } from 'axios';
// do this
import axios from 'axios';
import type { SomeAxiosType } from 'axios';

But I'll submit a fix as soon as some maintainer approves this issue.

[daolou]

+1

[matushorvath]

+1
This seems to have some unfortunate interaction with jest and underscore, where it causes ~25% of our repos to fail unit test compilation. Downgrading typescript to 3.8 removes the failures.

I managed to reduce it to a minimum jest test case that fails:

import * as underscore from 'underscore';

test('test', () => {
    underscore.uniq(['']);
});

With the output:

npx jest --config jest.json test/a.test.ts
 FAIL  test/a.test.ts
  ● Test suite failed to run

    TypeError: Cannot redefine property: default
        at Function.defineProperty (<anonymous>)



      at test/a.test.ts:10:12
      at __importStar (test/a.test.ts:18:5)
      at Object.<anonymous> (test/a.test.ts:1:1)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        8.556s

(Yes, the line number are wrong, I don't know why. The source file is just 6 lines.)
Debugging beyond this is more than I can do right now though, so I'm not even sure if this should be reported to jest or typescript project. FWIW, at least I'm posting my findings hoping that it might help someone.

[BenSjoberg]

Looks like tslib had the same issue: microsoft/tslib#102

They fixed it by reverting to the previous behavior in tslib 1.x, and publishing the breaking change as 2.0. This means that another workaround for this issue is to add tslib ^1.13.0 as a dependency, then set "importHelpers": true in tsconfig.json. (Note that I've only tested this on a very simple project, I'm not sure if using the old import helpers will break anything.)

If this is working as intended, I hope that it can be reverted in 3.x and pushed back to 4.0, since it's a breaking change.

[joeldenning]

We're also running into this with the launchdarkly react-client-sdk. See launchdarkly/react-client-sdk#36.

Here is a another simple demonstration of the issue: https://github.com/joeldenning/typescript-esm-cjs-interop

[DanielRosenwasser]

We should have a fix at #38808. If you can pick up the build that was produced here and try it out, that'd really help give us some confidence that we can back-port it to 3.9.

[BenSjoberg]

@DanielRosenwasser That build fixes the issue for me. Modules imported with the __importStar helper are no longer causing the "Cannot redefine property: default" error.

[joeldenning]

The fix for this is released in 3.9.4, which is currently released to npm under the dev dist-tag.