lukeed / ley

(WIP) Driver-agnostic database migrations
MIT License
260 stars 14 forks source link

ERR_REQUIRE_ESM with import of `*.js` from `ley.config.mjs` #35

Closed karlhorky closed 1 year ago

karlhorky commented 1 year ago

Hi @lukeed 👋 hope things are going well!

Encountered an unusual problem the other day:

  1. CommonJS project (no "type": "module" in package.json)
  2. ley.config.mjs file which imports a .js file, which also uses ESM
  3. ERR_REQUIRE_ESM error, trying to require() the ley.config.mjs file 🤔

CodeSandbox: https://codesandbox.io/p/sandbox/lucid-babycat-74sz59?file=%2Fley.config.mjs%3A3%2C19

$ ley up
node:internal/errors:490
    ErrorCaptureStackTrace(err);
    ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /workspace/ley.config.mjs not supported.
Instead change the require of /workspace/ley.config.mjs to a dynamic import() which is available in all CommonJS modules.
    at exports.load (/workspace/node_modules/ley/lib/util.js:62:10)
    at async /workspace/node_modules/ley/bin.js:11:13 {
  code: 'ERR_REQUIRE_ESM'
}

Node.js v18.16.0
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Screenshot 2023-07-25 at 19 30 47

I guess I would instead expect the error that Node.js throws here, complaining that A) it cannot find the named export and B) that ley-import-broken.js is a CommonJS module:

$ node ley.config.mjs 
file:///Users/k/p/a/ley.config.mjs:1
import { a } from './ley-import-broken.js';
         ^
SyntaxError: Named export 'a' not found. The requested module './ley-import-broken.js' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from './ley-import-broken.js';
const { a } = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:124:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:190:5)

Node.js v18.17.0

Somewhat related to this other issue by @mplibunao:

karlhorky commented 1 year ago

Ley's migrate up is however still failing, so having a different error message is not as much a real problem as it is confusing...

karlhorky commented 1 year ago

Ideally what I would love would be to have ley.config.js + all .js imports treated as ESM, even without "type": "module" in package.json (or maybe ley.config.ts supported, when tsm is used - which would also unlock ESM syntax)

But yeah, can also see downsides here...

lukeed commented 1 year ago

You can't do this for Node reasons, not ley. You can only (technically) have ESM within .js with "type":"module" in your pkg config. Thats how native loaders can know what to expect/allow. (Bundlers can be different story.. aka more permissive.) This was done because ESM was a later, additive feature, and .js had to remain IIFE/CommonJS-oriented by default... type: module turns on the ESM assumption.

The reason you're seeing ERR_REQUIRE_ESM is because import() is tried first, and falls back to require. Node is throwing an error (with a stderr warning, see CSB) and so the require() tried again, also throwing an error.

Screen Shot 2023-07-25 at 11 53 32 AM
karlhorky commented 1 year ago

Ok good, so it sounds like this is "by design" and Ley won't go in the direction of bundler behavior? If so, we can probably close this issue and leave it as being documented.

lukeed commented 1 year ago

It's just plain Node loading / ESM semantics. I don't think it needs documenting since it applies to every module (and .js file) within the ecosystem. There isn't anything ley specific about the situation.

thanks~!