--esModuleInterop changes meaning of declaration

[ghost]

TypeScript Version: 2.8.0-dev.20180201

Code

m.d.ts

declare function m(): void;
declare namespace m {}
export = m;

n.d.ts

import * as m from "./m";
export const n: typeof m;

a.ts

import { n } from "./n";
n();

Expected behavior:

No error.

Actual behavior:

No error normally. With tsc --esModuleInterop:

src/a.ts(2,1): error TS2349: Cannot invoke an expression whose type lacks a call signature. Type '{ default: () => void; }' has no compatible call signatures.
src/n.d.ts(1,1): error TS7038: A namespace-style import cannot be called or constructed, and will cause a failure at runtime.

The user may not have control over the definition file, so I think we could a) omit the error in the definition file, and b) preserve the original meaning where typeof m is callable.

[weswigham]

I'm not really a fan of eliding the error just because the source import is in a definition file. It's a legitimate shape (reexporting the namespace as a member) that would strip the call and construct signatures if the .d.ts truly reflects the source. I think doing this for compat reasons may hamstring our ability to catch real errors - each of these files could easily be generated from the user's own code and actually be indicative of an issue.

[weswigham]

@andy-ms You suggestion in the associated dt PR:

That should probably be import serveStatic = require("serve-static") in express/index.d.ts -- could you see if that works for you?

Is really the correct fix. You shouldn't use an es6-style import if you don't want es6-style semantics.

[mhegazy]

Chatted with @weswigham offline, we need to go update definitelytyped to remove all references of this form to avoid users having to deal with the error.

[mhegazy]

@weswigham can you run the tests and let us know which ones need updating? i would be happy to help fixing them as needed.

[weswigham]

I opened DefinitelyTyped/DefinitelyTyped#23355 with fixes for the packages that I identified had issues. I can't seem to load @andy-ms 's PR that enables the flag on all packages at DefinitelyTyped/DefinitelyTyped#23354 - it keeps Unicorn'ing.

[zspitz]

@weswigham If it helps, I was able to load the page in a Chrome incognito window; maybe not being logged into GitHub has some effect.

[Jessidhia]

It also would be great if we could get, in writing, in some DefinitelyTyped guidelines, that you always must write import x = require('x') in DefinitelyTyped code if the package export is commonjs.

As an example, a lot of the pull requests to @types/react-loadable involved people changing the require-imports into star imports and sometimes my request to change to require-import were ignored because "it works", or when they were done people would eventually revert it in some pull request I didn't get to review.

[aluanhaddad]

So is this then not considered a bug? I think this is good behavior as @weswigham stated.

[mhegazy]

Had a chat about this offline, now that DefinitelyTyped has been updated, we will add a new lint rule to ensure that reexports of an export = use import = require. We will not force the use of --esModuleInterop flag on DT packages in the time being.

[weswigham]

DTSLint is now enforcing this on DT, so I think we're good

[Jessidhia]

That's backwards; the problem is namespace imports, not default imports. If you do a default import, you'll just get a compile error; if you do a namespace import, you'll get a runtime error.

[weswigham]

The namespace imports are flagged in the type checker, as all packages now have esModuleInterop on by default. The lint rule is "backwards" to ensure that you produce a type definition file that also works for people not using allowSyntheticDefaultImports or esModuleInterop.

[domoritz]

Is https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/json-stable-stringify/index.d.ts correct?

[weswigham]

Yes. The (commonjs) package exports a single top-level function. You should import as import stringify = require("json-stable-stringify") (if targeting commonjs modules) or, if esModuleInterop and/or allowSyntheticDefaultimports is set, you may use import stringify from "json-stable-stringify"

[domoritz]

Hmm, when I use import stableStringify from 'json-stable-stringify' with "esModuleInterop": true (and allowSyntheticDefaultimports: false) I get this error

src/util.ts:1:8 - error TS1192: Module '"/Users/domoritz/Developer/UW/vega-lite/node_modules/@types/json-stable-stringify/index"' has no default export.

1 import stableStringify from 'json-stable-stringify';
         ~~~~~~~~~~~~~~~


error Command failed with exit code 1.

[weswigham]

I assume with module: es2015, yeah? We only consider esModuleInterop to imply allowSyntheticDefaultimports when the module target is commonjs, amd, umd, or system.

[domoritz]

Yes.

[weswigham]

Right, so you need to set allowSyntheticDefaultimports: true yourself to assert that whatever is running your modules will understand that you expect default imports for non-esm modules to exist.