Skip to content

Conversation

@kbarbounakis
Copy link
Contributor

@kbarbounakis kbarbounakis commented Feb 1, 2026

This pull request updates the build configuration to improve compatibility with ES modules by replacing usage of __dirname and introducing a new plugin dependency. The most important changes are grouped below:

Build configuration improvements:

  • Updated the Rollup configuration in rollup.config.js to use the rollup-plugin-replace plugin, replacing all occurrences of __dirname with import.meta?.dirname to better support ES module environments. [1] [2]
  • Added rollup-plugin-replace as a development dependency in package.json.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request attempts to improve ES module compatibility in src/SqliteExtensions.js by updating the path module import and adding support for __dirname in ES modules. However, the implementation contains critical bugs that will cause runtime errors.

Changes:

  • Changed from default path import to named resolve import
  • Added a __dirname definition to handle ES module environments
  • Updated all path resolution calls to use resolve directly

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 1 to 3
import {resolve} from 'path';
// __dirname is not available in ES modules, so we define it here
const __dirname = __dirname || import.meta.dirname;
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line has a logical error: const __dirname = __dirname || import.meta.dirname; references __dirname on the right side before it's defined. This will result in a ReferenceError when __dirname is not available (i.e., in ES modules).

For ES module compatibility, the correct approach is to use import.meta.url with the fileURLToPath and dirname functions from the 'path' and 'url' modules:

import { fileURLToPath } from 'url';
import { dirname } from 'path';
const __dirname = import.meta.url ? dirname(fileURLToPath(import.meta.url)) : __dirname;

Alternatively, if using Node.js 20.11.0 or later where import.meta.dirname is available, use a try-catch or conditional check that doesn't reference the undefined variable.

Suggested change
import {resolve} from 'path';
// __dirname is not available in ES modules, so we define it here
const __dirname = __dirname || import.meta.dirname;
import {resolve, dirname} from 'path';
import {fileURLToPath} from 'url';
// __dirname is not available in ES modules, so we define it here
const __dirname = typeof __dirname !== 'undefined'
? __dirname
: dirname(fileURLToPath(import.meta.url));

Copilot uses AI. Check for mistakes.
@@ -1,9 +1,11 @@
import path from 'path';
import {resolve} from 'path';
// __dirname is not available in ES modules, so we define it here
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment states that __dirname is not available in ES modules, but the code attempts to reference it before defining it. In CommonJS (the current module system used by this package according to package.json), __dirname IS available as a global. In true ES modules, __dirname would cause a ReferenceError.

This comment is misleading because:

  1. The source files use ES6 import/export syntax but are transpiled to CommonJS by Babel/Rollup
  2. When running in CommonJS (the default format), __dirname is already available
  3. When running as an ES module, import.meta.dirname may not be available in older Node.js versions (it was added in Node.js 20.11.0)

The comment should clarify the actual use case or be updated to reflect the correct fallback strategy.

Copilot uses AI. Check for mistakes.
Comment on lines 1 to 3
import {resolve} from 'path';
// __dirname is not available in ES modules, so we define it here
const __dirname = __dirname || import.meta.dirname;
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of import.meta.dirname requires Node.js 20.11.0 or later, but package.json specifies "engines": { "node": ">=12" }. This creates a version compatibility issue.

In Node.js versions 12-20.10.x, import.meta.dirname is undefined, which would cause the fallback logic to fail (even if the fallback logic itself weren't broken). Consider either:

  1. Using import.meta.url with fileURLToPath and dirname for broader compatibility
  2. Updating the minimum Node.js version requirement in package.json to 20.11.0 or later

Note that import.meta.dirname was only added in Node.js 20.11.0 (released January 2024) and 21.2.0.

Suggested change
import {resolve} from 'path';
// __dirname is not available in ES modules, so we define it here
const __dirname = __dirname || import.meta.dirname;
import {resolve, dirname} from 'path';
import {fileURLToPath} from 'url';
// __dirname is not available in ES modules, so we define it here
const __filename = fileURLToPath(import.meta.url);
const __dirname = dirname(__filename);

Copilot uses AI. Check for mistakes.
@kbarbounakis kbarbounakis merged commit 6e57a7d into themost-framework:master Feb 1, 2026
3 checks passed
@kbarbounakis kbarbounakis deleted the resolve-dirname-in-esm-modules branch February 1, 2026 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant