Skip to content
New issue

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

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

Already on GitHub? # to your account

CJS imported when ESM export available, then improperly mapped to esm (module.default became exported_module.default.default) #3580

Closed
BLucky-gh opened this issue Jan 5, 2024 · 9 comments

Comments

@BLucky-gh
Copy link

BLucky-gh commented Jan 5, 2024

In an esm project (type:module in package.json), when bundling @bogeychan/elysia-logger@0.0.14, which is an esm-only dependency, with format=esm and target=node20 the elysia@0.8.5 import gets bundled as (truncated)

// node_modules/.pnpm/elysia@0.8.5_@sinclair+typebox@0.32.5_typescript@5.3.3/node_modules/elysia/dist/cjs/index.js
var require_cjs2 = __commonJS({
  "node_modules/.pnpm/elysia@0.8.5_@sinclair+typebox@0.32.5_typescript@5.3.3/node_modules/elysia/dist/cjs/index.js"(exports) {
    "use strict";
    var __importDefault2 = exports && exports.__importDefault || function(mod) {
      return mod && mod.__esModule ? mod : { "default": mod };
    };
    Object.defineProperty(exports, "__esModule", { value: true });
    exports.InvalidCookieSignature = exports.InternalServerError = exports.ValidationError = exports.NotFoundError = exports.ParseError = exports.error = exports.getResponseSchemaValidator = exports.mergeObjectArray = exports.mergeHook = exports.mergeDeep = exports.getSchemaValidator = exports.Cookie = exports.t = exports.mapEarlyResponse = exports.mapCompactResponse = exports.mapResponse = exports.Elysia = void 0;
    var memoirist_1 = require_cjs();
    var eventemitter3_1 = __importDefault2(require_eventemitter3());
    var trace_1 = require_trace();
    var ws_1 = require_ws();
    var handler_1 = require_handler();
    var compose_1 = require_compose();
    var utils_1 = require_utils();
    var dynamic_handle_1 = require_dynamic_handle();
    var error_1 = require_error3();
    var type_system_1 = require_type_system();
    var Elysia5 = class _Elysia { //truncated here for brevity



// more code


   exports.default = Elysia5;
    exports.Elysia = Elysia5;
    var handler_2 = require_handler();
    Object.defineProperty(exports, "mapResponse", { enumerable: true, get: function() {
      return handler_2.mapResponse;
    } });
    Object.defineProperty(exports, "mapCompactResponse", { enumerable: true, get: function() {
      return handler_2.mapCompactResponse;
    } });
    Object.defineProperty(exports, "mapEarlyResponse", { enumerable: true, get: function() {
      return handler_2.mapEarlyResponse;
    } });
    var type_system_2 = require_type_system();
    Object.defineProperty(exports, "t", { enumerable: true, get: function() {
      return type_system_2.t;
    } });
    var cookie_1 = require_cookie2();
    Object.defineProperty(exports, "Cookie", { enumerable: true, get: function() {
      return cookie_1.Cookie;
    } });
    var utils_2 = require_utils();
    Object.defineProperty(exports, "getSchemaValidator", { enumerable: true, get: function() {
      return utils_2.getSchemaValidator;
    } });
    Object.defineProperty(exports, "mergeDeep", { enumerable: true, get: function() {
      return utils_2.mergeDeep;
    } });
    Object.defineProperty(exports, "mergeHook", { enumerable: true, get: function() {
      return utils_2.mergeHook;
    } });
    Object.defineProperty(exports, "mergeObjectArray", { enumerable: true, get: function() {
      return utils_2.mergeObjectArray;
    } });
    Object.defineProperty(exports, "getResponseSchemaValidator", { enumerable: true, get: function() {
      return utils_2.getResponseSchemaValidator;
    } });
    var error_2 = require_error3();
    Object.defineProperty(exports, "error", { enumerable: true, get: function() {
      return error_2.error;
    } });
    Object.defineProperty(exports, "ParseError", { enumerable: true, get: function() {
      return error_2.ParseError;
    } });
    Object.defineProperty(exports, "NotFoundError", { enumerable: true, get: function() {
      return error_2.NotFoundError;
    } });
    Object.defineProperty(exports, "ValidationError", { enumerable: true, get: function() {
      return error_2.ValidationError;
    } });
    Object.defineProperty(exports, "InternalServerError", { enumerable: true, get: function() {
      return error_2.InternalServerError;
    } });
    Object.defineProperty(exports, "InvalidCookieSignature", { enumerable: true, get: function() {
      return error_2.InvalidCookieSignature;
    } });
  }
});

// snip
var import_elysia = __toESM(require_cjs2(), 1);
//snip

which lines up with /elysia/dist/cjs/index.js as seen on https://www.npmjs.com/package/elysia?activeTab=code, as opposed to /elysia/dist/index.js that is already available

This in turn, results in the error "TypeError: import_elysia.default is not a constructor" down the line when trying to execute the bundled equivalent of this code,

function into(options = {}) {
  const autoLogging = options.autoLogging ?? true;
  delete options.autoLogging;
  let log;
  let app3 = new import_elysia.default({
    name: "@bogeychan/elysia-logger",

which I assume could be related to webpack/webpack#706 (even though that one is a webpack issue), which is supported by the fact that editing let app3 = new import_elysia.default({ to let app3 = new import_elysia.default.default({ fixes the issue

@BLucky-gh
Copy link
Author

BLucky-gh commented Jan 5, 2024

oh righjt, forgot to mention

esbuild version 0.18.13
built on github actions default runner, so x86 ubuntu probably

@evanw
Copy link
Owner

evanw commented Jan 5, 2024

Please follow the issue reporting instructions:

When reporting a bug or requesting a feature, please do the following:

  • Provide a way to reproduce the issue. The best way to do this is to demonstrate the issue on the playground (https://esbuild.github.io/try/) and paste the URL here. A link to a minimal code sample with instructions for how to reproduce the issue may also work. Issues without a way to reproduce them may be closed.

Marking this issue as unactionable because there's nothing I can do with it.

@BLucky-gh
Copy link
Author

I was hoping the exact version specifiers would be enough context, but if you need an actual example file, I can make a minimal repro project tomorrow

@hyrious
Copy link

hyrious commented Jan 5, 2024

The entry point from elysia is CommonJS because it has such package.json:

  "exports": {
    ".": {
      "bun": "./dist/bun/index.js",
      "node": "./dist/cjs/index.js",    // <-- choose this one when `import "elysia"`
      "require": "./dist/cjs/index.js",
      "import": "./dist/index.js",
      "default": "./dist/index.js"

@BLucky-gh
Copy link
Author

The entry point from elysia is CommonJS because it has such package.json:

  "exports": {
    ".": {
      "bun": "./dist/bun/index.js",
      "node": "./dist/cjs/index.js",    // <-- choose this one when `import "elysia"`
      "require": "./dist/cjs/index.js",
      "import": "./dist/index.js",
      "default": "./dist/index.js"

that makes sense, but is there a way to override that for esbuild?

also when I was making the reproduction I noticed that the error that appeared before I finished adding all the files indicated that esbuild was trying to look for what's in the import key in the package.json, and not the node key

also that still leaves out the problem that esbuild bundles the default under module.default.default but tries to access it under module.default

(actual reproduction coming in a bit)

@BLucky-gh
Copy link
Author

@evanw
Here's the reproduction:
https://github.com/BLucky-gh/esbuild-bug-repro-3580

just do a pnpm build or node ./build.js and then try running dist/main.js

@BLucky-gh
Copy link
Author

Small update: externalizing elysia and installing it separately does make esbuild properly insert the import Elysia from "elysia" and the let app3 = new Elysia(, but trying to run it still leads to the error

❯ node packages/back/api-gateway/src/main.mjs
file:///<path truncated>/src/main.mjs:22462
  let app3 = new Elysia({
             ^

TypeError: Elysia is not a constructor
    at Pino.into (file:///<path truncated>/src/main.mjs:22462:14)
    at plugin (file:///<path truncated>/src/main.mjs:22488:94)
    at logger (file:///<path truncated>/src/main.mjs:22443:10)
    at file:///<path truncated>/src/main.mjs:38234:30

and replacing let app3 = new Elysia( with let app3 = new Elysia.default( fixes the issue again, even though this is still supposed to be an import handled by node, so there might be an issue with how the cjs was bundled for elysia.

I'll keep this issue open just in case, but if you look a the repro and see nothing wrong on the esbuild site you can close this issue

@evanw
Copy link
Owner

evanw commented Jan 6, 2024

The entry point from elysia is CommonJS because it has such package.json:

  "exports": {
    ".": {
      "bun": "./dist/bun/index.js",
      "node": "./dist/cjs/index.js",    // <-- choose this one when `import "elysia"`
      "require": "./dist/cjs/index.js",
      "import": "./dist/index.js",
      "default": "./dist/index.js"

that makes sense, but is there a way to override that for esbuild?

I'm assuming node is active because you have set the platform to node, which is because you are running the result in node. In this case, the package author for elysia has deliberately chosen that CommonJS should always be imported in node. I'm not quite sure why but sometimes it's necessary to do this for correctness. If you don't do this, you are open to the dual package hazard which can cause your library to be unintentionally instantiated twice which can introduce bugs. So it would be incorrect for esbuild to do anything other than follow the author's intent here.

If you believe the author has made a mistake in their package configuration, you are welcome to contact them and try to get it changed. You can also write an esbuild plugin to override esbuild's behavior if you think it's really necessary, although keep in mind that you may be opening yourself up to bugs later on if you do that.

also when I was making the reproduction I noticed that the error that appeared before I finished adding all the files indicated that esbuild was trying to look for what's in the import key in the package.json, and not the node key

That's because esbuild only looks for the node key when the platform is set to node. The default platform is browser, not node. This is expected behavior.

replacing let app3 = new Elysia( with let app3 = new Elysia.default( fixes the issue

You may find this helpful: https://esbuild.github.io/content-types/#default-interop. In node, importing the export named default import of a CommonJS file from an ESM file (i.e. a .js file in a package with "type": "module") returns module.exports instead of module.exports.default. This is a huge pain for compatibility with the rest of the JavaScript ecosystem but that's just how they have decided it should work and they are very unlikely to change it given that it would be a breaking change. The reason why esbuild is doing this too is detailed in that link but basically this is the behavior that you get when you run that code in node, so esbuild is faithfully replicating what would happen in node. I consider esbuild's behavior to be correct here.

I notice all code examples on https://elysiajs.com/ use a named import (import { Elysia } from 'elysia') instead of a default import (import Elysia from 'elysia'). You may want to try to get that problematic code changed to avoid the default import. It looks like someone has already proposed this: elysiajs/elysia#396.

I'm going to close this issue because everything esbuild is doing here seems correct to me. It's respecting the package author's wishes and faithfully replicating the behavior of running the code in node itself.

@evanw evanw closed this as not planned Won't fix, can't repro, duplicate, stale Jan 6, 2024
@evanw evanw removed the unactionable label Jan 6, 2024
@BLucky-gh
Copy link
Author

thanks for the detailed response, I opened an issue on elysia detailing this issue and a pull request on elysia-logger that replaces the default import with the named one

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants