Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esModuleInterop is not supported #1112

Open
rubenlg opened this issue Nov 7, 2019 · 12 comments
Open

esModuleInterop is not supported #1112

rubenlg opened this issue Nov 7, 2019 · 12 comments
Labels
internal-issue-created Google internal issue has been created for this triage-done

Comments

@rubenlg
Copy link
Contributor

rubenlg commented Nov 7, 2019

When I run tsickle with esModuleInterop, tsickle produces the wrong javascript, with a regular require instead of goog.require.

Repro steps:

npm install typescript@~3.5.3 tsickle@0.37.0

I wish I could use tsickle 0.37.1, but unfortunally the npm package doesn't have a binary (or a main.js file).

tsconfig.json:

{
  "compilerOptions": {
    "target": "es2017",
    "module": "commonjs",
    "strict": true,
    "esModuleInterop": true,
  }
}

a.ts:

import * as b from './b';

b.foo();

b.ts:

export function foo() {}

Generated a.js:

goog.module('a');
var module = module || { id: 'a.ts' };
const tsickle_b_1 = goog.requireType("b");
const b = __importStar(require("./b"));
b.foo();

If I don't use import * as ... then the emitted JS is correct.

esModuleInterop is now one of the default flags generated when running tsc --init.

@evmar
Copy link
Contributor

evmar commented Nov 7, 2019

Tsickle is now only a library, no binary:
#1075
See also the updated front-page docs on how to use it.

@rubenlg
Copy link
Contributor Author

rubenlg commented Nov 8, 2019

Thanks @evmar. I tried with tsickle 0.37.1 (using demo.js) and Typescript 3.6.4, and I still reproduce the error.

@mprobst
Copy link
Contributor

mprobst commented Nov 11, 2019

@rubenlg I think what Evan is saying is that demo is not expected to cover all use cases, and you're expected to create your own driver app (or use tscc) that can then set the flags correctly.

@evmar
Copy link
Contributor

evmar commented Nov 11, 2019

I left this one open because I think the bug is right, we just don't support esModuleInterop. One fix might be for us to check for flags like these and error on start.

@rubenlg
Copy link
Contributor Author

rubenlg commented Nov 11, 2019

Somehow my comment disappeared from the thread. I tried tscc and got the same error.

Note that esModuleInterop is now one of the default flags when you use tsc --init, so this is going to be frustrating for people starting to use tsickle for the first time. Erroring on start for unsupported tsconfig flags sounds like a good first step.

@mprobst
Copy link
Contributor

mprobst commented Nov 12, 2019

Internal issue created: b/144351394.

@mprobst mprobst added internal-issue-created Google internal issue has been created for this triage-done labels Nov 12, 2019
@theseanl
Copy link
Contributor

theseanl commented Nov 12, 2019

I would prefer to support --esModuleInterop in tscc. It only affects behavior for importing non-ts modules (commonjs, amd, umd), so if tsickle assumes that all the modules will be provided to tsickle (which seems to be the primary use case, since it already modifies every require calls to goog.require calls) the presence of the flag does not make any runtime difference.

However, it makes difference to tscc in that it allows writing namespace import statements for external modules in a default export statement, which I think is a good thing.

Therefore, I hope tsickle does not error out on --esModuleInterop. There are some alternative things that tsickle may do:

  • If tsickle is expected to never emit bare require() calls in future, it may simply strip out top-level __importStar and __importDefault calls in googmodule transformer, which will make no runtime difference due to the assumption, and it also looks reasonable since it already have codes to transform __exportStar calls. I think tscc can simply remove .default access of external module namespaces.

  • If emitting require() call is in the future scope, then tsickle may check that the imported source is provided to the typescript compiler and strip __import** calls only in such cases.

  • If tsickle decides not to support --esModuleInterop, it would still be possible for tscc to support it if this behavior can be configured via an API. In this case the logic to remove __import** calls needs to be added to tscc.

@rubenlg
Copy link
Contributor Author

rubenlg commented Nov 12, 2019

@theseanl When you say that it only afffects behavior for importing non-ts modules, what do you mean exactly?

In the example I provided above, both a.ts and b.ts are regular Typescript modules. When you run tsickle (via tscc or otherwise) on that code without --esModuleInterop it compiles file. But when --esModuleInterop is present (which is now the default), the import of b.ts from a.ts is broken.

If we were to drop the __importStar line, closure would not be able to process b.foo() because it doesn't know which module is b coming from.

Rewriting the __importStar line from tscc into a correct goog.require is certainly possible. I created a tsickle driver that does that by running a regexp right before writing the JS file. But it's also quite hacky and brittle.

@theseanl
Copy link
Contributor

When you say that it only afffects behavior for importing non-ts modules, what do you mean exactly?

I meant that when it is transpiled using tsc and ran on a JS engine the behavior is identical.

I meant by "stripping out __importStar calls" /__importStar\((.*)\)$/$1/ or something like that.

@rubenlg
Copy link
Contributor Author

rubenlg commented Nov 12, 2019

Thanks for clarifying! Makes sense now.

@Zamiell
Copy link

Zamiell commented Apr 1, 2020

Any update on this issue? I'm sure tons of people out there would love to use tsickle with their TypeScript projects, but not supporting "esModuleInterop" seems to be a show-stopper.

In my particular case, my project has ES6-style import statements on JSON files, so it requires "esModuleInterop" to be set to true or it won't even compile. As far as I know, this kind of this is the recommended solution / best practice for TypeScript, e.g. the highest upvoted answer here: https://stackoverflow.com/a/50830840

@evmar
Copy link
Contributor

evmar commented Apr 1, 2020

If you import JSON files, it won't compile under JSCompiler anyway, so I think this bug in particular isn't blocking you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal-issue-created Google internal issue has been created for this triage-done
Projects
None yet
Development

No branches or pull requests

5 participants