lukeed / ley

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

Rethrow error if not ESM import error #36

Open karlhorky opened 9 months ago

karlhorky commented 9 months ago

Hey @lukeed, hope you're good!

Today when running ley new we received an error with ERR_REQUIRE_ESM:

This PR instead rethrows the error if it's not an error related to importing the ESM file (inspired by similar code in tsm)


In the meantime, while this PR is waiting on a review, we've published our fork here:

karlhorky commented 9 months ago

Ah, I think ERR_REQUIRE_ESM from my new code is the wrong error code - I guess it's the opposite error code from what we want...

codecov-commenter commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 49.54%. Comparing base (0cf486e) to head (c218b21).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #36 +/- ## ========================================== + Coverage 48.82% 49.54% +0.71% ========================================== Files 2 2 Lines 213 220 +7 ========================================== + Hits 104 109 +5 - Misses 109 111 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

karlhorky commented 9 months ago

Ok, was a bit more complicated than matching a single error code, but it seems to be working now, and not too much code:

// Rethrow error if it is not any of:
// - not found error
// - old Node.js compatibility error
if (e && e.message && !e.code === 'ERR_MODULE_NOT_FOUND' &&
  !['Not supported', 'require(...).pathToFileURL is not a function'].includes(e.message)
) throw e;
karlhorky commented 7 months ago

@lukeed friendly ping, have any thoughts on this one?

karlhorky commented 5 months ago

@lukeed Ah just found a logic error in my implementation, here's the fixed version from the last commits:

        // Rethrow error if it is not any of:
        // - not found error
        // - old Node.js compatibility error
        if (e && e.message &&
-           !e.code === 'ERR_MODULE_NOT_FOUND' &&
+           e.code !== 'ERR_MODULE_NOT_FOUND' &&
            !['Not supported', 'require(...).pathToFileURL is not a function'].includes(e.message)
        ) throw e;
karlhorky commented 5 months ago

Edit: outdated - see my comment below: I worked around this with an additional check for a /migrations/*.ts path in the error message

Show outdated comment content ❗️(large) Caveat: this disables support for TypeScript migration files when using `--require tsm` or `--require tsx/cjs`: ```bash $ pnpm migrate up > next-js-example-winter-2024-atvie@0.1.0 migrate /Users/k/projects/next-js-example-winter-2024-atvie > ley --require tsx/cjs "up" [ley] Loading configuration [ley] An error occurred: TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /Users/k/projects/next-js-example-winter-2024-atvie/migrations/00000-createTableAnimals.ts at Object.getFileProtocolModuleFormat [as file:] (node:internal/modules/esm/get_format:160:9) at defaultGetFormat (node:internal/modules/esm/get_format:203:36) at defaultLoad (node:internal/modules/esm/load:143:22) at async ModuleLoader.load (node:internal/modules/esm/loader:409:7) at async ModuleLoader.moduleProvider (node:internal/modules/esm/loader:291:45) at async link (node:internal/modules/esm/module_job:76:21) code: ERR_UNKNOWN_FILE_EXTENSION  ELIFECYCLE  Command failed with exit code 1. ``` ## Workaround Instead of `ley --require tsm`, use `NODE_OPTIONS='--import tsx' ley` (on non-Windows machines), as proposed here: - #37
karlhorky commented 5 months ago

Since Windows does not support npm scripts setting environment variables NODE_OPTIONS in the same way as macOS and Linux, I'll add an additional condition to handle this ERR_UNKNOWN_FILE_EXTENSION error code instead.

karlhorky commented 4 months ago

@lukeed we revisited this today because a student was having problems with the condition still swallowing the error on the following import:

import { setEnvironmentVariables } from 'util/config';

This led to the following confusing ESM error (we use "type": "module" in our projects and the fallback require() in Ley cannot require() an ES Module):

$ pnpm ley new createUsers.ts --esm
C:\Users\HUAWEI\projects\books-inspo-app\books-inspo\node_modules\@upleveled\ley\lib\util.js:71
                return require(id);
         ^

Error [ERR_REQUIRE_ESM]: require() of ES Module C:\Users\HUAWEI\projects\books-inspo-app\books-inspo\ley.config.js from C:\Users\HUAWEI\projects\books-inspo-app\books-inspo\node_modules\@upleveled\ley\lib\util.js not supported.
Instead change the require of ley.config.js in C:\Users\HUAWEI\projects\books-inspo-app\books-inspo\node_modules\@upleveled\ley\lib\util.js to a dynamic import() which is available in all CommonJS modules.
    at exports.load (C:\Users\HUAWEI\projects\books-inspo-app\books-inspo\node_modules\@upleveled\ley\lib\util.js:71:10)
    at async C:\Users\HUAWEI\projects\books-inspo-app\books-inspo\node_modules\@upleveled\ley\bin.js:11:13 {
  code: 'ERR_REQUIRE_ESM'
}

Node.js v20.11.1

But the real error was actually the missing .js in the fully-specified import path - should have been this:

import { setEnvironmentVariables } from 'util/config.js';

Without this error swallowing behavior of Ley, the error message would have been the following, which is much more helpful:

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/Users/lukas/Documents/UpLeveled/2024winter/julia/books-inspo-app/books-inspo/util/config' imported from /Users/lukas/Documents/UpLeveled/2024winter/julia/books-inspo-app/books-inspo/ley.config.js
Did you mean to import ../util/config.js?
    at finalizeResolution (node:internal/modules/esm/resolve:264:11)
    at moduleResolve (node:internal/modules/esm/resolve:917:10)
    at defaultResolve (node:internal/modules/esm/resolve:1130:11)
    at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:396:12)
    at ModuleLoader.resolve (node:internal/modules/esm/loader:365:25)
    at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:240:38)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:85:39)
    at link (node:internal/modules/esm/module_job:84:36) {
  code: 'ERR_MODULE_NOT_FOUND',
  url: 'file:///Users/lukas/Documents/UpLeveled/2024winter/julia/books-inspo-app/books-inspo/util/config'
}

For this reason, I simplified the rethrow condition (and fixed the test) in c218b21f0d06fbf25443945f80d9e3525bb18969

If you would have an opportunity to review this PR and offer some feedback, it would be great!