esModuleInterop does not handle named imports when combined with synthetic default imports

[frankwallis]

TypeScript Version: 2.8.0-dev.20180204
Search Terms: esModuleInterop
Code

I am seeing some unexpected behaviour when using esModuleInterop and importing from a commonjs module in 2 different ways at the same time:

// dep.js
exports.var1 = 'var1';
exports.var2 = 'var2';

import Deps, { var2 } from './dep';
console.log(Deps.var1);
console.log(var2);

Expected behaviour:
Running the code outputs:

var1
var2

Actual behaviour:
Running the code outputs:

var1
undefined

This is the javascript which is generated:

"use strict";
var __importDefault = (this && this.__importDefault) || function (mod) {
    return (mod && mod.__esModule) ? mod : { "default": mod };
}
exports.__esModule = true;
var dep_1 = __importDefault(require("./dep"));
console.log(dep_1["default"].var1);
console.log(dep_1.var2);

Related Issues:
None that I am aware of

The real-world code which is triggering this problem is here

The 'named' import and 'default' import work fine individually but do not work when they are both present in the same file.

[Jessidhia]

This is working as intended, then. import { var2 } from './dep' imports the var2 export, but there is no var2 export as it is not an ES module export.

CommonJS modules only have a default export. exports.var2 = is not a var2 export, it is the var2 property of the default export.

Code that did import { Component } from 'react' has always been wrong and only worked by coincidence.

[weswigham]

@Kovensky This is not intended. We should be emitting the __importStar and not the __importDefault helper when the import form is import D, { ... }.

Specifically:

CommonJS modules only have a default export. exports.var2 = is not a var2 export, it is the var2 property of the default export.

This is still under discussion is by no means set yet (we're watching it carefully). While it will be available as the default (which is what makes the flag needed), most transpilers today also allow named accesses of commonjs modules; and the goal of this flag is mostly to align with other transpilers and give people a path to migrate forward (since TS'd usual behavior wouldn't allow that)

@frankwallis as a workaround, if you split up the import into default/not default import statements it should work until we issue a patch.

[aluanhaddad]

But looking at React specifically, we see https://github.com/facebook/react/blob/master/packages/react/src/React.js#L33

The top level package exports a single binding. All of what have been used as named imports are properties of this export.

And then there is this great comment.

[frankwallis]

React does not set the __esModule flag so it will be seen as a normal commonjs module. Typescript is then converting it to an object with a default property and trying to access other named imports from that object.

https://unpkg.com/react@16.2.0/cjs/react.development.js

[aluanhaddad]

Yeah what I was getting at is that it seems anything but clear how the package intends to be consumed. With no __esModule flag I don't see how it is intended to work with named exports even if you discount the fact that Node.js doesn't support them. On the other hand, there is so much production code and heaps of documentation suggesting use of named imports from React.

@weswigham

This is still under discussion is by no means set yet (we're watching it carefully).

I really hope things do change but it is easy to get the impression that Node.js is supremely disinterested in these compatibility scenarios.

[frankwallis]

@aluanhaddad as I understand it (and @weswigham indicated) - even if nodejs does not itself support named imports, they will still be supported when using typescript or babel. The code you would need to write will be automatically generated based upon the value of the __esModule flag.

[aluanhaddad]

@frankwallis thank you for clarifying that. But since React doesn't set the flag, I would still argue that

import React, Component from 'react';

even if supported, in transpilation, should be avoided because React does not seem at all interested in supporting it. Granted, that is a philosophical argument.

I've been recommending

import React from 'react';
const {Component} = React;

Regarding named imports in Node.js, it implies a perpetual transpilation step even where not otherwise needed. Users seeing NodeJS advertising module support in Node.js 10, may be confused or surprised, unaware that they have taken such a dependency. More problematically, if they are authoring libraries, their documentation may not reflect the larger Node.js community's expectations.

I'm not trying to say what ought to happen here, but rather that these usage patterns emerged at a time when Node.js was not expected to take the course they now have.

As @Kovensky has noted many times, both TypeScript and Babel are highly permissive. I believe a strict conformance mode would be beneficial if only to set a baseline expectation of behavior in TypeScript. This whole space is currently very error prone. It is very easy to make mistakes and not even realize it until you are consumed by some complex downstream pipeline.

[weswigham]

I believe a strict conformance mode would be beneficial if only to set a baseline expectation of behavior in TypeScript.

Right. But whose strict behavior? Node doesn't know how the feature will stabilize yet, and browsers don't support any cross module-kind interop at all (yet do seem somewhat open to opening up their loader more in the future). As it turns out, es modules "arrived" with es6, but were the least standard new feature in the standard, since many behaviors are left unspecified. Browsers operate without interop, so how TS works today without a flag can emulate browser behavior pretty well (excepting the fact that declaration files allow you to confuse module kinds). The esmoduleinterop flag allows TS to emulate Babel's and webpack's interop behaviors, which, as should be obvious, many people have come to rely on. Whatever the node team finally lands on, we'll support; but I think the devs are doing a huge disservice to their existing community who was told they were using "es6 modules" with their transpiled code where they would "just be able to disable Babel" in the future when the JavaScript of the future arrives by not meeting developers where they are. They're clearly aware of this - they've introduced exceptions to the only-default approach for the builtin packages like fs and path, so they clearly know how awful it is right now (though such exceptions are gonna make our life more difficult).

Really, we were all sold on the lie that we could reasonably smoothly transition into these things called modules without actually knowing how they'd behave in some very important cases. I'm a bit biased because I'm immersed in how the feature has evolved, but I'd not advocate for using modules with the intent of writing future-looking code anymore. Use them if you only want to target one runtime, or never remove a transpiler from your toolchain, but don't decieve yourself into thinking they'll ever be as portable as a chunk of code with a UMD definition block around it was in the last era of the web.

[frankwallis]

Please can this issue be included in one of the upcoming milestones, or be opened to the community? Currently it causes a runtime error with no compiler error when consuming (for example) antd via its es-modules entry point.

[ds300]

It would be great to get some clarity on this from the TS team.

Here it is labelled as a bug, but in #22678 @RyanCavanaugh said it's the expected behaviour.

The current behaviour is extremely surprising, and, AFAICT, should be considered a bug.

i.e. Why should

import _, { blah } from 'thing'
console.log(blah)

and

import _ from 'thing'
import { blah } from 'thing'
console.log(blah)

produce different results? Because that's what's happening right now if an imported module doesn't specify __esModule: true

I'm happy to contribute a fix for this, if there's agreement on what the behaviour should be.

[weswigham]

It's definitely a bug. People are getting hung up on tangential issues. We're just not counting the named imports for an importStar helper after we see the default import.

[rdsedmundo]

I'm still having this problem. Not sure if it's the same thing, can you confirm? @weswigham

Compile error:

import API from 'apifn';

// TS2351: Cannot use 'new' with an expression whose type lacks a call or construct signature.
export default new API('https://google.com', []);

This works:

import { default as API } from 'apifn';

export default new API('https://google.com', []);

Both allowSyntheticDefaultImports and esModuleInterop are set to true here.

I can't understand how those syntaxes are different, as I believe the default import syntax is part of the TC39 standard already or isn't it? Asking because this works perfectly with Babel. Only when I'm with projects with TypeScript that this confusion with default starts to happen.

The apifn package is a small package developed by myself. It uses TypeScript for generating ES5+Common JS.

[weswigham]

Definitely unrelated to this issue - this issue only affected emit, while that's a typechecking error.

[rafsawicki]

I'm still having this error when using esModuleInterop with dynamoose library and Typescript 2.9.2 / 3.0.0-dev.20180704 (current @next).

When I use only named imports it works just fine

import { Schema } from 'dynamoose';

console.log(Schema)    // constructor function

On the other hand, if I use default import, named import changes to undefined

import dynamoose, { Schema } from 'dynamoose';

console.log(Schema)   // Schema is undefined
console.log(dynamoose.Schema)    // constructor function

Both of these examples work with Babel 6.24 so I don't think it's correct behavior. It compiles just fine, but this could be a weakness of type definitions.

For reference, in first example the generated code looks like this

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
const dynamoose_1 = require("dynamoose");
console.log(dynamoose_1.Schema);

In second case the code generated looks like this

"use strict";
var __importStar = (this && this.__importStar) || function (mod) {
    if (mod && mod.__esModule) return mod;
    var result = {};
    if (mod != null) for (var k in mod) if (Object.hasOwnProperty.call(mod, k)) result[k] = mod[k];
    result["default"] = mod;
    return result;
};
Object.defineProperty(exports, "__esModule", { value: true });
const dynamoose_1 = __importStar(require("dynamoose"));
console.log(dynamoose_1.Schema);
console.log(dynamoose_1.default.Schema);

And that's what Babel generates

'use strict';

var _dynamoose = require('dynamoose');

var _dynamoose2 = _interopRequireDefault(_dynamoose);

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

console.log(_dynamoose.Schema);
console.log(_dynamoose2.default.Schema);

[Jessidhia]

The second one (where Schema is undefined) is the correct one.

This is how dynamoose exports itself: https://github.com/dynamoosejs/dynamoose/blob/master/lib/index.js#L125

This should be defined as an export = commonjs export.

[rafsawicki]

Even if dynamoose export is technically incorrect, it's still very strange that adding new import type breaks the one used previously. If using named import isn't correct in this case, shouldn't it's value also be undefined in example 1 - when used without default import?

I may be mistaken in thinking that esModuleInterop would bring TS behavior in line with the way that Babel works, but even ignoring Babel, current behavior is still puzzling...

[frankwallis]

This is an interesting one, I think in this situation it is because Schema is defined on the prototype of Dynamoose, not the actual object:

Dynamoose.prototype.Schema = Schema;
Dynamoose.prototype.Table = require('./Table');
Dynamoose.prototype.Dynamoose = Dynamoose;

module.exports = new Dynamoose();

Because typescript is using importStar here Schema is not copied over to the synthesized module because it fails the hasOwnProperty test. Either that or it should generate the same code for both console.log lines, I'm not sure.

[rafsawicki]

@frankwallis Do you think it's worth opening a new issue describing this problem?

[frankwallis]

@rafsawicki I do believe this is a new issue, maybe @weswigham can comment?

[AlexGalays]

Shouldn't esmoduleinterop make it possible to do

import { useState } from 'react' ?

It's very annoying to use right now.

[weswigham]

@AlexGalays that's probably just waiting on an updated react declaration file on definitely typed.

[AlexGalays]

Oh no, I meant it doesn't work at runtime. Nevermind it works now... debugging with approximative source maps were showing useState as undefined but it wasn't.

[JeroMiya]

FYI for those who got here from a google search seeing this error in intellisense in VS2019 despite their create-react-app based project building fine from the command line, I the error to go away by doing this (not sure if all these steps were necessary):

• Cleaned solution
• Closed solution
• Deleted bin/obj folders
• Did a clean yarn install (deleted node_modules)
• Opened solution again
• Problem went away (for me at least)

It tends to happen to me whenever I move a component into a different directory using the Solution Explorer. Sometimes just closing and re-opening the project makes it go away and sometimes I have to do the full clean/close/install/reopen dance.