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

Incorrect import() range for Node matrix #1772

Closed
lukeed opened this issue Nov 13, 2021 · 2 comments
Closed

Incorrect import() range for Node matrix #1772

lukeed opened this issue Nov 13, 2021 · 2 comments

Comments

@lukeed
Copy link
Contributor

lukeed commented Nov 13, 2021

I think this is a two-part issue.

For starters, I have this as for my config:

esbuild.transform({
  // ...
  target: 'node' + process.versions.node,
  format: 'esm',
})

And then here's a basic example file:

import('dset').then(console.log);
export const foo = 123;

Ran this in Node 12.20+ (tested 12.22.7), which means I have target: "node12.22.7" active.

The first problem here is that esbuild's internal matrix thinks that this version (and presumably all 12.20+) doesn't support import(), but it does.

The second is that because esbuild thinks node12.22.7 doesn't support import(), it gets rewritten into a require statement, even though it's still exporting ESM syntax:

// target=node12.22.7; format=esm
var __create = Object.create;
var __defProp = Object.defineProperty;
var __getOwnPropDesc = Object.getOwnPropertyDescriptor;
var __getOwnPropNames = Object.getOwnPropertyNames;
var __getProtoOf = Object.getPrototypeOf;
var __hasOwnProp = Object.prototype.hasOwnProperty;
var __markAsModule = (target) => __defProp(target, "__esModule", { value: true });
var __reExport = (target, module, desc) => {
  if (module && typeof module === "object" || typeof module === "function") {
    for (let key of __getOwnPropNames(module))
      if (!__hasOwnProp.call(target, key) && key !== "default")
        __defProp(target, key, { get: () => module[key], enumerable: !(desc = __getOwnPropDesc(module, key)) || desc.enumerable });
  }
  return target;
};
var __toModule = (module) => {
  return __reExport(__markAsModule(__defProp(module != null ? __create(__getProtoOf(module)) : {}, "default", module && module.__esModule && "default" in module ? { get: () => module.default, enumerable: true } : { value: module, enumerable: true })), module);
};
Promise.resolve().then(() => __toModule(require("dset"))).then(console.log);
const foo = 123;
export {
  foo
};

This throws a ReferenceError: require is not defined

If I go through and hardcode a target: 'es2015' (or later), then it'll work as expected:

// target=es2015; format=esm
import("dset").then(console.log);
const foo = 123;
export {
  foo
};

So the TLDR for the 2nd issue here is that format isn't applying to dynamic imports, which allows an output targeting ESM to (unknowingly) include require statements.

@lukeed lukeed changed the title Output target is overriding format value Incorrect import() range for Node matrix Nov 13, 2021
@lukeed
Copy link
Contributor Author

lukeed commented Nov 13, 2021

Test these Node versions for dynamic import support:

  • 12.14.1 ❌
  • 12.15.0 ❌
  • 12.16.3 ❌
  • 12.17.0 ⚠️
  • 12.18.3 ⚠️
  • 12.18.4 ⚠️
  • 12.19.1 ⚠️
  • 12.20.2 ✅

Warning disappears @ 12.20

@rdmurphy
Copy link

rdmurphy commented Nov 14, 2021

I see that this isn't the first time this has been raised, and in the past it was left as Node 13.2 because "Node version 12 is no longer active." But that's not true according to Node's LTS schedule:

image

It's v13 that's no longer actively being maintained by Node (nor v15!), not v12, because v12 still has maintenance LTS status through April 2022. (You can also see this reflected in the version support matrix for AWS Lambda.) I'd bet that it's much more likely for a version of Node 12 to still be out in the wild than Node 13. To me that's an argument for considering 12.20 the true (or at least most useful) beginning of dynamic import support.

There were only three releases of Node 13 that didn't support it:

13.0.0
13.0.1
13.1.0
13.2.0 <-- dynamic import supported --
13.3.0
13.4.0
13.5.0
13.6.0
13.7.0
13.8.0
13.9.0
13.10.0
13.10.1
13.11.0
13.12.0
13.13.0
13.14.0

Node 12 had 34:

12.0.0
12.1.0
12.2.0
12.3.0
12.3.1
12.4.0
12.5.0
12.6.0
12.7.0
12.8.0
12.8.1
12.9.0
12.9.1
12.10.0
12.11.0
12.11.1
12.12.0
12.13.0
12.13.1
12.14.0
12.14.1
12.15.0
12.16.0
12.16.1
12.16.2
12.16.3
12.17.0
12.18.0
12.18.1
12.18.2
12.18.3
12.18.4
12.19.0
12.19.1
12.20.0 <-- dynamic import supported --
12.20.1
12.20.2
12.21.0
12.22.0
12.22.1
12.22.2
12.22.3
12.22.4
12.22.5
12.22.6
12.22.7

Because it's not possible to have support start and stop across major versions, IMO 12.20 is a much more useful place to start.

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

No branches or pull requests

2 participants