fix(new-compiler): use dynamic import for lmdb to avoid CJS transform errors#2015
Merged
AndreyHirsa merged 1 commit intomainfrom Feb 24, 2026
Merged
fix(new-compiler): use dynamic import for lmdb to avoid CJS transform errors#2015AndreyHirsa merged 1 commit intomainfrom
AndreyHirsa merged 1 commit intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughA patch release for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/new-compiler/src/metadata/manager.ts (1)
21-32:⚠️ Potential issue | 🟡 MinorModule-load failures get a misleading error label.
await import("lmdb")(andfs.mkdirSync) share the samecatchblock that always emits"Failed to open LMDB at ${dbPath}". If lmdb fails to load (e.g., native binding ABI mismatch), the outer label incorrectly implies a DB-open failure. The rootmessageis forwarded so it's still diagnosable, but it slows debugging.🔍 Proposed fix: distinguish import failure from open failure
async function openDatabaseConnection(dbPath: string, noSync: boolean): Promise<RootDatabase> { - try { - fs.mkdirSync(dbPath, { recursive: true }); - const { open } = await import("lmdb"); - return open({ - path: dbPath, - compression: true, - noSync, - }); - } catch (error) { - const message = error instanceof Error ? error.message : String(error); - throw new Error(`Failed to open LMDB at ${dbPath}: ${message}`); - } + fs.mkdirSync(dbPath, { recursive: true }); + let open: (typeof import("lmdb"))["open"]; + try { + ({ open } = await import("lmdb")); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + throw new Error(`Failed to load lmdb module: ${message}`); + } + try { + return open({ + path: dbPath, + compression: true, + noSync, + }); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + throw new Error(`Failed to open LMDB at ${dbPath}: ${message}`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new-compiler/src/metadata/manager.ts` around lines 21 - 32, The current try/catch wraps both fs.mkdirSync and await import("lmdb") so import failures get mislabeled as "Failed to open LMDB"; split the operations and add distinct error handling: perform fs.mkdirSync(dbPath, { recursive: true }) first, then wrap await import("lmdb") in its own try/catch and throw a clear "Failed to load lmdb module: <error>" message if it fails, and finally call open({ path: dbPath, compression: true, noSync }) in a separate try/catch that throws "Failed to open LMDB at ${dbPath}: <error>" on failure; reference the import("lmdb"), open({...}), fs.mkdirSync, dbPath and noSync identifiers when making these localized error messages.
🧹 Nitpick comments (1)
packages/new-compiler/src/metadata/manager.ts (1)
23-23: Optional: cache the resolvedopenreference across calls.While Node.js caches the module after the first
await import("lmdb"), a new microtask is scheduled on every invocation. A module-level lazy singleton makes the caching explicit and eliminates that overhead.♻️ Proposed refactor: lazy-cached import
+let _open: (typeof import("lmdb"))["open"] | undefined; +async function getLmdbOpen(): Promise<(typeof import("lmdb"))["open"]> { + if (!_open) { + ({ open: _open } = await import("lmdb")); + } + return _open; +} async function openDatabaseConnection(dbPath: string, noSync: boolean): Promise<RootDatabase> { try { fs.mkdirSync(dbPath, { recursive: true }); - const { open } = await import("lmdb"); + const open = await getLmdbOpen(); return open({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new-compiler/src/metadata/manager.ts` at line 23, The code currently does an await import("lmdb") inside the function, scheduling a microtask each call; introduce a module-level lazy singleton (e.g., a cachedOpen or getOpen reference) to store the resolved open function and reuse it across invocations; change usages that currently do const { open } = await import("lmdb") to retrieve the cached open (initializing it on first call) so subsequent calls reuse the same open reference without re-importing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/new-compiler/src/metadata/manager.ts`:
- Around line 21-32: The current try/catch wraps both fs.mkdirSync and await
import("lmdb") so import failures get mislabeled as "Failed to open LMDB"; split
the operations and add distinct error handling: perform fs.mkdirSync(dbPath, {
recursive: true }) first, then wrap await import("lmdb") in its own try/catch
and throw a clear "Failed to load lmdb module: <error>" message if it fails, and
finally call open({ path: dbPath, compression: true, noSync }) in a separate
try/catch that throws "Failed to open LMDB at ${dbPath}: <error>" on failure;
reference the import("lmdb"), open({...}), fs.mkdirSync, dbPath and noSync
identifiers when making these localized error messages.
---
Nitpick comments:
In `@packages/new-compiler/src/metadata/manager.ts`:
- Line 23: The code currently does an await import("lmdb") inside the function,
scheduling a microtask each call; introduce a module-level lazy singleton (e.g.,
a cachedOpen or getOpen reference) to store the resolved open function and reuse
it across invocations; change usages that currently do const { open } = await
import("lmdb") to retrieve the cached open (initializing it on first call) so
subsequent calls reuse the same open reference without re-importing.
cherkanovart
approved these changes
Feb 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Use dynamic import for lmdb to fix SyntaxError: 'super' keyword unexpected here caused by bundlers/require hooks transforming lmdb's CJS bundle.
Changes
Testing
Business logic tests added:
Visuals
N/A - no UI changes.
Checklist
Summary by CodeRabbit
Bug Fixes