salsita/node-pg-migrate

Incorrect reference to __dirname when compiling to esmodules

PatKayongo opened this issue · 4 comments

Describe the bug

When running the migrations using the Programmatic API, the node-pg-migrate source has a reference to __dirname which is not available when using ES modules.

Steps to reproduce

When running the migrations using the Programmatic API using ES modules as follows:

import nodePostgres from 'pg';
import migrationRunner from 'node-pg-migrate';
import { fileURLToPath } from 'url';
import path from 'path';
const { Pool } = nodePostgres;
export const initialiseDatabase = async (databaseConfig) => {
    const pool = new Pool({
        host: databaseConfig.host,
        database: databaseConfig.database,
        port: databaseConfig.port,
        password: databaseConfig.password,
        user: databaseConfig.username,
        ssl: { rejectUnauthorized: false },
    });
    const migrationsPath = path.join(path.dirname(fileURLToPath(import.meta.url)), 'migrations');
    const migrationClient = await pool.connect();
    try {
        await migrationRunner({
            dbClient: migrationClient,
            migrationsTable: 'database_migrations',
            dir: migrationsPath,
            direction: 'up',
            noLock: true,
        });
        migrationClient.release();
    }
    catch (error) {
        migrationClient.release();
        throw error;
    }
    return pool;
};

The following error is returned:

Error: Can't get migration files: ReferenceError: __dirname is not defined

Looking at the source file, it looks like there is a reference to __dirname, which is only accessible using CommonJS modules.

Logs

Error: Can't get migration files: ReferenceError: __dirname is not defined
at file:///path/to/project/node_modules/node-pg-migrate/dist/esm/index.mjs:2579:115
at Array.map ()
at loadMigrations (file:///path/to/project/node_modules/node-pg-migrate/dist/esm/index.mjs:2577:13)
at async Promise.all (index 0)
at async runner (file:///path/to/project/node_modules/node-pg-migrate/dist/esm/index.mjs:2763:36)
at async initialiseDatabase (file:///path/to/project/dist/infrastructure/postgres/index.mjs:19:9)
at async file:///path/to/project/dist/index.mjs:13:26
at loadMigrations (file:///path/to/project/node_modules/node-pg-migrate/dist/esm/index.mjs:2601:11)
at async Promise.all (index 0)
at async runner (file:///path/to/project/node_modules/node-pg-migrate/dist/esm/index.mjs:2763:36)
at async initialiseDatabase (file:///path/to/project/dist/infrastructure/postgres/index.mjs:19:9)
at async file:///path/to/project/dist/index.mjs:13:26

System Info

System:
    OS: macOS 14.4.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 59.11 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.11.1 - ~/.nvm/versions/node/v20.11.1/bin/node
    Yarn: 3.2.0 - ~/.nvm/versions/node/v20.11.1/bin/yarn
    npm: 10.2.4 - ~/.nvm/versions/node/v20.11.1/bin/npm

Used Module System

esm

This should be catchable with a lint rule

This should be catchable with a lint rule

What do you mean?
__dirname is legit in node-pg-migrate's codebase cause it was historically fully written only with cjs in mind.
Everything here is valid and over in #1168 we are already working on a fix.
However, we need to ensure compatibility to cjs and esm for the same time to ensure a smooth migration between major versions.

Ah right, since it needs to support both, then just banning one style or the other doesn’t make as much sense.

That said as a pattern I’ve had some success in other contexts using linting as a detection tool - ban both patterns, but then selectively add lint ignore comments with explanations ensure that the places where the library does need to use them have been exhaustively audited.

That lets the tooling guide us to detect missed cases and avoid regressions.

Just a note, I do not believe #1206 will resolve this issue. Please see my evaluation of the experimental release which was intended to resolve #1168 (which was somewhat subsumed by #1187).