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 reference to __dirname when compiling to esmodules #1167

Open
PatKayongo opened this issue May 13, 2024 · 3 comments · May be fixed by #1168
Open

Incorrect reference to __dirname when compiling to esmodules #1167

PatKayongo opened this issue May 13, 2024 · 3 comments · May be fixed by #1168
Assignees
Labels
c: bug Something isn't working p: 1-normal Nothing urgent
Milestone

Comments

@PatKayongo
Copy link

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

@PatKayongo PatKayongo added the s: pending triage Pending Triage label May 13, 2024
@Shinigami92 Shinigami92 linked a pull request May 13, 2024 that will close this issue
@Shinigami92 Shinigami92 added c: bug Something isn't working p: 1-normal Nothing urgent and removed s: pending triage Pending Triage labels May 13, 2024
@Shinigami92 Shinigami92 added this to the v7.x milestone May 13, 2024
@osdiab
Copy link
Collaborator

osdiab commented May 30, 2024

This should be catchable with a lint rule

@Shinigami92
Copy link
Collaborator

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.

@osdiab
Copy link
Collaborator

osdiab commented May 31, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants