-
Notifications
You must be signed in to change notification settings - Fork 51
fix: dbt commands fail with ESM SyntaxError since v0.5.3
#407
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
043c7a9
fix: dbt commands fail with `SyntaxError: Cannot use import statement…
anandgupta42 afc7a10
fix: address code review — idempotency guard and test assertion accuracy
anandgupta42 a152425
test: adversarial e2e tests for dbt-tools ESM loading under Node
anandgupta42 210a95f
fix: address PR review — strengthen test assertions per CodeRabbit
anandgupta42 7e4182c
Merge branch 'main' into fix/dbt-tools-esm-package-json
anandgupta42 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
125 changes: 125 additions & 0 deletions
125
packages/opencode/test/install/dbt-tools-bundle.test.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| import { describe, test, expect } from "bun:test" | ||
| import fs from "fs" | ||
| import path from "path" | ||
|
|
||
| const REPO_ROOT = path.resolve(import.meta.dir, "../../../..") | ||
| const PUBLISH_SCRIPT = path.join(REPO_ROOT, "packages/opencode/script/publish.ts") | ||
| const DBT_TOOLS_DIR = path.join(REPO_ROOT, "packages/dbt-tools") | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 1. Source dbt-tools uses ESM — if this changes, publish.ts must adapt | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| describe("dbt-tools ESM contract", () => { | ||
| test('source package.json declares "type": "module"', () => { | ||
| const pkg = JSON.parse(fs.readFileSync(path.join(DBT_TOOLS_DIR, "package.json"), "utf-8")) | ||
| expect(pkg.type).toBe("module") | ||
| }) | ||
|
|
||
| test("bin/altimate-dbt uses node shebang (not bun)", () => { | ||
| const bin = fs.readFileSync(path.join(DBT_TOOLS_DIR, "bin/altimate-dbt"), "utf-8") | ||
| expect(bin).toContain("#!/usr/bin/env node") | ||
| }) | ||
|
|
||
| test("bin/altimate-dbt uses ESM import() to load dist/index.js", () => { | ||
| const bin = fs.readFileSync(path.join(DBT_TOOLS_DIR, "bin/altimate-dbt"), "utf-8") | ||
| expect(bin).toContain('import("../dist/index.js")') | ||
| }) | ||
|
|
||
| test("build outputs ESM format (--format esm in build script)", () => { | ||
| const pkg = JSON.parse(fs.readFileSync(path.join(DBT_TOOLS_DIR, "package.json"), "utf-8")) | ||
| const buildScript = pkg.scripts?.build || "" | ||
| expect(buildScript).toContain("--format esm") | ||
| }) | ||
| }) | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 2. publish.ts must bundle a package.json with "type": "module" | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| describe("publish.ts dbt-tools ESM bundling", () => { | ||
| const publishSource = fs.readFileSync(PUBLISH_SCRIPT, "utf-8") | ||
|
|
||
| test("copyAssets writes a dbt-tools/package.json", () => { | ||
| // The publish script must create a package.json in the bundled dbt-tools dir. | ||
| // Without it, Node defaults to CJS and `import` statements in dist/index.js fail. | ||
| expect(publishSource).toContain("dbt-tools/package.json") | ||
| }) | ||
|
|
||
| test('bundled package.json includes "type": "module"', () => { | ||
| // The publish script must write `{ "type": "module" }` (or equivalent) | ||
| // so that Node treats .js files in dbt-tools/ as ESM. | ||
| expect(publishSource).toContain('"type": "module"') | ||
| }) | ||
|
|
||
| test("copyAssets creates dbt-tools/bin and dbt-tools/dist directories", () => { | ||
| expect(publishSource).toContain("dbt-tools/bin") | ||
| expect(publishSource).toContain("dbt-tools/dist") | ||
| }) | ||
|
|
||
| test("copyAssets copies the altimate-dbt bin wrapper", () => { | ||
| expect(publishSource).toContain("dbt-tools/bin/altimate-dbt") | ||
| }) | ||
|
|
||
| test("copyAssets copies dist/index.js (not the entire dist/ tree)", () => { | ||
| // Copying all of dist/ would include ~220MB of .node native binaries | ||
| expect(publishSource).toContain("dist/index.js") | ||
| }) | ||
| }) | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 3. Structural: if dbt-tools ever switches away from ESM, tests should catch it | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| describe("dbt-tools + Node compatibility", () => { | ||
| test("bin wrapper import path matches dist output location", () => { | ||
| // The bin wrapper does `import("../dist/index.js")`. | ||
| // If the build output location changes, this test forces an update. | ||
| const bin = fs.readFileSync(path.join(DBT_TOOLS_DIR, "bin/altimate-dbt"), "utf-8") | ||
| const match = bin.match(/import\(["']([^"']+)["']\)/) | ||
| expect(match).not.toBeNull() | ||
| const importPath = match![1] | ||
| expect(importPath).toBe("../dist/index.js") | ||
| }) | ||
|
|
||
| test("Node would fail without package.json type:module (regression guard)", () => { | ||
| // This is the exact scenario that caused the bug: | ||
| // - bin/altimate-dbt has `#!/usr/bin/env node` | ||
| // - It uses `import("../dist/index.js")` | ||
| // - dist/index.js starts with `import { createRequire } from "node:module"` | ||
| // - Without "type": "module" in package.json, Node treats .js as CJS | ||
| // - CJS cannot use top-level `import` → SyntaxError | ||
| // | ||
| // This test verifies the chain of conditions that require package.json: | ||
| const bin = fs.readFileSync(path.join(DBT_TOOLS_DIR, "bin/altimate-dbt"), "utf-8") | ||
|
|
||
| // Condition 1: Uses node (not bun) as the runtime | ||
| const usesNode = bin.includes("#!/usr/bin/env node") | ||
| // Condition 2: Uses ESM import syntax | ||
| const usesESMImport = bin.includes("import(") | ||
|
|
||
| if (usesNode && usesESMImport) { | ||
| // Then package.json MUST have "type": "module" | ||
| const pkg = JSON.parse(fs.readFileSync(path.join(DBT_TOOLS_DIR, "package.json"), "utf-8")) | ||
| expect(pkg.type).toBe("module") | ||
|
|
||
| // AND publish.ts MUST bundle that information | ||
| const publishSource = fs.readFileSync(PUBLISH_SCRIPT, "utf-8") | ||
| expect(publishSource).toContain('"type": "module"') | ||
| } | ||
| }) | ||
|
|
||
| test("all dbt-tools bin entries use node shebang (consistency check)", () => { | ||
| const pkg = JSON.parse(fs.readFileSync(path.join(DBT_TOOLS_DIR, "package.json"), "utf-8")) | ||
| const binEntries: Record<string, string> = pkg.bin || {} | ||
|
|
||
| for (const [name, relPath] of Object.entries(binEntries)) { | ||
| const binPath = path.join(DBT_TOOLS_DIR, relPath) | ||
| expect(fs.existsSync(binPath)).toBe(true) | ||
|
|
||
| const content = fs.readFileSync(binPath, "utf-8") | ||
| // All bin entries should use node (not bun) since end users may not have bun | ||
| expect(content).toContain("#!/usr/bin/env node") | ||
| } | ||
| }) | ||
| }) | ||
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.