-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Incorrect CJS import with esmoduleInterop: true
#1971
Comments
This behavior is aligned to babel since 0.14.5. Which means: if a cjs module defined You can still switch to the node behavior by renaming your entry file to end with |
Then I guess the TS types are incorrect, since we should presumably get an error there if the default export doesn't exist? But shouldn't esbuild be creating the default export for us, considering we're using |
I think you are mis-understanding what |
But from my experience it also changes how default imports work - if it's false you have to use My main concern is that there should be some error somewhere along the line here to indicate that something has gone awry, and will crash in runtime - but I have no idea where it would go |
I can't reproduce this using tsc: // a.ts
import core from "@actions/core"
console.log(core.info) // expect: [Function: info] tsc a.ts --esModuleInterop // a.js
"use strict";
var __importDefault = (this && this.__importDefault) || function (mod) {
return (mod && mod.__esModule) ? mod : { "default": mod };
};
exports.__esModule = true;
var core_1 = __importDefault(require("@actions/core"));
console.log(core_1["default"].info); // expect: [Function: info] node a.js TypeError: Cannot read properties of undefined (reading 'info') Notice the error message actually matches the babel behavior: the default export is always |
Sorry, I meant the type checking changes to allow it. I did not know what the code output would so I didn't mean to make it sound like that was what I was referring to. But if that's where the error is then I should probably open a ticket there then. It would be nice with a warning if you imported |
Well, the type checking is not totally wrong. As I said before this is correct if you are running this code in esm format in node. (e.g. You name it with '.mjs' extension or add 'type: module' in package.json and run it with node directly). This is definitely some sort of historical issue when the idea of 'es modules' firstly introduced in typescript. My suggestion is, platform matters how they excutes. You can and have to follow one of the cjs-to-esm rule in esbuild. Other bundlers may not give you such choice. |
I wrote up some more documentation about esbuild's specific behavior regarding
|
Great writeup on the problem, glad this is being so clearly communicated. I just wanted to share a brief note here on a hack that eg JSPM uses for this - When importing an ES module from a CJS module: const dep = require('dep');
// this is the particular interop pattern that causes the issue
const depDefault = dep.__esModule ? dep.default : dep; is transformed into: import * as depNs from 'dep';
// try fetch the nested default if it exists (for support with Node.js interop), falling back to "babel interop"
const dep = depNs.default || depNs;
// this pattern now works for both CJS + ESM imports / Node.js or Babel interop
const depDefault = dep.__esModule ? dep.default : dep; This does in turn have another tradeoff that an ES module with a default and named exports cannot properly be imported into CommonJS. But I would argue the problem is not so much the I hope it's the same right interop discussion on the above, but let me know if I've missed any details too. |
Repro: https://github.com/BeeeQueue/esbuild-cjs-no-default
TL;DR:
import core from "@actions/core"
generates invalid code which fails when running, presumably because it does something funky with the exports somewhere.I tried to look around a bit to see if I could find what they're doing weird but couldn't find anything myself.
It can be worked around with
import * as
, but if that is the intended behavior there should probably be a warning about it.Maybe related to #1079?
The text was updated successfully, but these errors were encountered: