Fix JS build to use AML in CLI #339
Conversation
WalkthroughThis pull request introduces several changes across various files, primarily focusing on dependency management and import statement adjustments. A new dependency, Changes
Possibly related PRs
Warning Rate limit exceeded@loicknuchel has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
libs/connector-mongodb/package.json (1)
8-8: Consider adding a comment about the module system choice.Since this is part of a larger architectural decision to use CommonJS modules, it would be helpful to document the reasoning.
Add a comment above the type field:
+ // Using CommonJS for better compatibility with CLI and existing Node.js ecosystem "type": "commonjs",libs/utils/package.json (1)
8-8: Consider ESM migration in the future.While CommonJS is valid and works well, consider planning a future migration to ECMAScript Modules (ESM) as it's becoming the standard in the JavaScript ecosystem. This would provide better tree-shaking, static analysis capabilities, and better alignment with modern JavaScript practices.
libs/connector-oracle/package.json (1)
8-8: Consider documenting the module system decision.Since this is part of a broader standardization to use CommonJS across packages, it would be helpful to document this decision in the project's technical documentation.
Consider adding a note in the README or contributing guidelines about the project's module system strategy and why CommonJS was chosen.
libs/connector-postgres/package.json (1)
8-8: Document the module system requirement.Consider adding a note in the package's README about the CommonJS module system requirement to help consumers understand potential integration considerations.
#!/bin/bash # Check if README exists and contains module system information if fd README.md --type f --exec grep -l "commonjs" {} > /dev/null; then echo "Module system already documented in README" else echo "Consider adding module system documentation" filibs/connector-mariadb/package.json (1)
8-8: Consider documenting the module system choice.Since this is a significant configuration affecting how the package is consumed, consider adding a note about the module system choice in the package's README.
Example documentation:
+ ## Module System + + This package uses CommonJS modules (`"type": "commonjs"` in package.json) to maintain compatibility with the CLI integration and existing Node.js ecosystem. When importing this package, use the CommonJS require syntax or namespace imports in TypeScript.cli/src/convert.ts (1)
70-70: Ensure consistent error handlingThe error message "AML generator not available" should match the format used in parseDialect for consistency. Consider using ParserResult.failure for both cases.
- if (dialect === 'aml') return Result.failure('AML generator not available') // FIXME: return Result.success(generateAml(db)) + if (dialect === 'aml') return Result.failure('AML generator not available: ESM compatibility issue') // FIXME: return Result.success(generateAml(db))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
cli/package.json(1 hunks)cli/src/convert.ts(2 hunks)cli/src/index.ts(1 hunks)cli/tsconfig.json(0 hunks)extensions/desktop/package.json(1 hunks)extensions/vscode-aml/package.json(1 hunks)extensions/vscode-aml/src/web/convert.ts(1 hunks)extensions/vscode-aml/src/web/diagnostics.ts(1 hunks)extensions/vscode-aml/src/web/extension.ts(1 hunks)extensions/vscode-aml/src/web/new.ts(1 hunks)extensions/vscode-aml/src/web/open.ts(1 hunks)extensions/vscode-aml/src/web/preview.ts(1 hunks)extensions/vscode-aml/src/web/utils.ts(1 hunks)extensions/vscode-aml/tsconfig.json(1 hunks)gateway/package.json(1 hunks)libs/aml/rollup.config.ts(0 hunks)libs/connector-bigquery/package.json(1 hunks)libs/connector-couchbase/package.json(1 hunks)libs/connector-mariadb/package.json(1 hunks)libs/connector-mongodb/package.json(1 hunks)libs/connector-mysql/package.json(1 hunks)libs/connector-oracle/package.json(1 hunks)libs/connector-postgres/package.json(1 hunks)libs/connector-snowflake/package.json(1 hunks)libs/connector-sqlserver/package.json(1 hunks)libs/models/package.json(1 hunks)libs/parser-dbml/rollup.config.ts(0 hunks)libs/parser-prisma/rollup.config.ts(0 hunks)libs/parser-sql/rollup.config.ts(0 hunks)libs/utils/package.json(1 hunks)
💤 Files with no reviewable changes (5)
- cli/tsconfig.json
- libs/aml/rollup.config.ts
- libs/parser-prisma/rollup.config.ts
- libs/parser-sql/rollup.config.ts
- libs/parser-dbml/rollup.config.ts
✅ Files skipped from review due to trivial changes (10)
- gateway/package.json
- libs/models/package.json
- libs/connector-sqlserver/package.json
- libs/connector-snowflake/package.json
- libs/connector-bigquery/package.json
- extensions/vscode-aml/src/web/open.ts
- extensions/vscode-aml/src/web/new.ts
- extensions/vscode-aml/src/web/extension.ts
- extensions/vscode-aml/src/web/convert.ts
- extensions/vscode-aml/src/web/utils.ts
🔇 Additional comments (12)
extensions/vscode-aml/src/web/diagnostics.ts (1)
1-2: LGTM! Import change aligns with VSCode extension best practices.
The change from default import to namespace import for the vscode module is correct and follows VSCode extension best practices.
Let's verify if this import pattern is consistently applied across the extension:
✅ Verification successful
Import pattern is consistently applied across the extension
The verification shows that all files in the extension use the namespace import pattern import * as vscode from "vscode". No default imports were found, confirming that the codebase follows a consistent and recommended approach for VSCode extension development.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent vscode import patterns across the extension
# Expected: All files should use namespace import pattern
# Search for any remaining default imports of vscode
echo "Checking for default imports..."
rg "import\s+vscode\s*[,}]" extensions/vscode-aml/
# Search for namespace imports to confirm consistency
echo "Checking namespace imports..."
rg "import\s+\*\s+as\s+vscode\s+from\s+['\"]vscode['\"]" extensions/vscode-aml/
Length of output: 895
libs/connector-mongodb/package.json (1)
8-8: LGTM! Verify consistency across related packages.
The addition of "type": "commonjs" is appropriate for standardizing the module system.
Let's verify this change is consistent across all related packages:
✅ Verification successful
✅ Module type configuration is consistent across all connector packages
All connector packages (bigquery, couchbase, mariadb, mongodb, mysql, oracle, postgres, snowflake, sqlserver) have the same "type": "commonjs" configuration in their package.json files, ensuring consistent module system usage across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all connector packages have the same module type configuration
# Expected: All connector packages should have "type": "commonjs"
# Search for package.json files in connector directories and check their type field
fd package.json libs/connector- --exec jq -r '. | select(.type != null) | "\(.name): \(.type)"'
Length of output: 200
Script:
#!/bin/bash
# Let's first find the correct paths for connector packages
fd -t d "connector-" libs/
# Then check package.json files in those directories
fd package.json libs/ -t f | grep "connector-" | xargs cat
Length of output: 10018
extensions/vscode-aml/tsconfig.json (2)
3-3: LGTM! Module system and path changes are well-aligned
The changes to use commonjs and explicit relative paths (./src, ./dist) are good practices:
- CommonJS aligns with the broader standardization effort across the project
- Explicit relative paths improve clarity and prevent potential path resolution issues
Also applies to: 6-7
4-5: Verify VSCode extension compatibility with ES2021
The upgrade to ES2021 for both lib and target might affect VSCode extension compatibility. Let's verify the minimum supported Node.js version in VSCode extensions.
✅ Verification successful
ES2021 configuration is compatible with VSCode extension requirements
The VSCode extension requires version ^1.95.0 (from package.json), which fully supports ES2021. Additionally, the use of ES2021 and WebWorker in the lib configuration is consistent with other TypeScript configurations in the project, where ES2021 is commonly used as the target. The configuration is appropriate for a VSCode extension and aligns with the project's standardization efforts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check VSCode extension manifest for engine requirements
# and compare with Node.js ES2021 compatibility
# Check if package.json exists and contains engine requirements
fd -t f "package.json" extensions/vscode-aml -x cat {} | jq -r '.engines | select(.vscode)'
Length of output: 119
Script:
#!/bin/bash
# Let's check VSCode's official documentation or configuration files for Node.js/ES compatibility
fd -t f "package.json" . -x cat {} | grep -A 5 -B 5 '"engines":'
# Also check if there are any other tsconfig files in the project for comparison
fd -t f "tsconfig.json" -x cat {}
Length of output: 11288
libs/connector-couchbase/package.json (1)
8-8: LGTM! Verify build system compatibility.
The addition of "type": "commonjs" aligns with the PR objective of fixing the JS build system. This change ensures consistent module resolution across the package ecosystem.
Let's verify the module system configuration across related packages:
✅ Verification successful
LGTM! Module system configuration is consistent across all connector packages
The verification confirms that all connector packages (@azimutt/connector-*) have the same "type": "commonjs" configuration, which aligns with the PR objective of fixing the JS build system. This consistent configuration ensures proper module resolution across the entire connector ecosystem.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent module system configuration across related packages
# Expected: All connector packages should have "type": "commonjs"
# Check for package.json files in libs/connector-* directories
fd -t f "package.json" "libs/connector-" -x jq -r '. | select(.type != null) | "\(.name): \(.type)"'
# Verify if any connector packages are missing the type field
fd -t f "package.json" "libs/connector-" -x jq -r '. | select(.type == null) | "\(.name): missing type field"'
Length of output: 413
Script:
#!/bin/bash
# Let's first check if the directory structure is different
fd -t d "connector"
# Then check package.json files in any directory containing "connector"
fd -t f "package.json" | grep -i "connector" | xargs cat
# As a backup, let's also check the repository structure
ls -R | grep ":$"
Length of output: 22186
libs/connector-oracle/package.json (1)
8-8: LGTM! Verify module resolution across dependencies.
The addition of "type": "commonjs" aligns with the standardization of module systems across packages.
Let's verify the module system consistency across related packages:
✅ Verification successful
The initial results show mixed module patterns: the package is set to CommonJS but has ESM-style imports in TypeScript files. We need to verify the TypeScript configuration to understand if this is handled by the TypeScript compiler configuration.
Module system configuration is correctly aligned
The configuration is consistent and well-structured:
- Package.json correctly declares
"type": "commonjs" - TypeScript is configured with
"module": "commonjs"and"esModuleInterop": true - ESM-style imports in TypeScript files are expected and will be transpiled to CommonJS by the TypeScript compiler
- All connector packages consistently use CommonJS, while some other packages use ESM where appropriate
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check module type configuration across all related packages
# Expected: All related packages should have consistent module type settings
# Check package.json files in libs and cli directories
fd package.json libs cli -x jq -r '. | select(.type != null) | "\(.name): \(.type)"'
# Check for any potential ESM syntax in source files that might conflict
rg -l "import \{.*\} from" libs/connector-oracle/
Length of output: 1215
Script:
#!/bin/bash
# Let's check if the TypeScript configuration is aligned with CommonJS
fd tsconfig.json libs/connector-oracle/ -x cat {}
# Also check if there are any CommonJS require statements to ensure consistency
rg -l "require\(['\"]" libs/connector-oracle/
Length of output: 606
libs/connector-mysql/package.json (1)
8-8: LGTM! Verify build configuration compatibility.
The addition of "type": "commonjs" aligns with the project-wide standardization of module systems across connectors.
Let's verify the build configuration compatibility:
✅ Verification successful
Potential module system conflict detected - needs attention
The verification revealed that several packages in the codebase are explicitly using ESM ("type": "module"), including critical dependencies:
libs/aml/package.jsonlibs/parser-sql/package.jsonlibs/parser-dbml/package.jsonlibs/parser-prisma/package.jsoncli/package.json
Let's verify if the MySQL connector has dependencies on these ESM packages:
Setting "type": "commonjs" is appropriate for this package
The verification confirms this change is correct because:
- The MySQL connector has no direct dependencies on any of the ESM packages
- Its dependencies (
@azimutt/models,@azimutt/utils,mysql2) are compatible with CommonJS - No imports from ESM packages were found in the codebase
The explicit CommonJS configuration will ensure proper module resolution and compatibility with the package's dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check build configuration across related files to ensure CommonJS compatibility
# Check if there are any ESM specific configurations that might conflict
rg -g '*.json' -g '*.js' -A 5 '"type":\s*"module"'
# Check for any rollup/webpack configurations that might need adjustment
fd -e config.js -e config.mjs -e config.cjs rollup webpack -x rg -l 'commonjs|requireReturnsDefault|@rollup/plugin-commonjs'
# Look for any potential module resolution issues in TypeScript config
fd tsconfig.json -x cat {} \; | jq -r '. | select(.compilerOptions.module != null) | .compilerOptions.module'
Length of output: 2987
Script:
#!/bin/bash
# Check dependencies of the MySQL connector
cat libs/connector-mysql/package.json | jq -r '.dependencies, .devDependencies, .peerDependencies | keys[]' 2>/dev/null | sort -u
# Check if any of the ESM packages are imported in the MySQL connector
rg -g '*.ts' -g '*.js' -l "from ['\"]([@/].*?/)(aml|parser-sql|parser-dbml|parser-prisma)" libs/connector-mysql/
Length of output: 336
libs/connector-postgres/package.json (1)
8-8: LGTM! Verify module system compatibility.
The addition of "type": "commonjs" aligns with the PR objective to fix the JS build system. Let's verify the compatibility with dependent packages.
✅ Verification successful
Let me gather more information about the specific module type used in the packages marked with "Different type" to ensure compatibility.
Module system configuration is compatible
The verification shows that connector-postgres and its direct workspace dependencies (@azimutt/models and @azimutt/utils) all use "type": "commonjs", ensuring compatibility. The presence of ESM packages in other parts of the monorepo (like cli, parser-*, and aml) doesn't affect this package as they are not direct dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all workspace dependencies have consistent module system settings
# Find all package.json files in the workspace
echo "Checking module system configuration across workspace packages..."
fd package.json --type f --exec sh -c '
if grep -q "\"type\": \"commonjs\"" {}; then
echo "✓ CommonJS: {}"
elif grep -q "\"type\":" {}; then
echo "⚠ Different type: {}"
else
echo "? No type specified: {}"
fi
'
Length of output: 1562
Script:
#!/bin/bash
# Check the specific module type for packages with different type
echo "Checking specific module types..."
for f in $(fd package.json --type f); do
if grep -q "\"type\":" "$f"; then
echo "=== $f ==="
grep "\"type\":" "$f"
fi
done
# Check if connector-postgres has dependencies on any non-commonjs packages
echo -e "\nChecking connector-postgres dependencies..."
cat libs/connector-postgres/package.json | jq -r '.dependencies + .devDependencies | keys[]' | while read dep; do
if [[ $dep == @azimutt/* ]]; then
pkg_path="libs/${dep#@azimutt/}/package.json"
if [ -f "$pkg_path" ]; then
echo "Dependency: $dep"
grep "\"type\":" "$pkg_path" || echo "No type specified"
fi
fi
done
Length of output: 7524
libs/connector-mariadb/package.json (1)
8-8: LGTM! Verify module resolution across the codebase.
The addition of "type": "commonjs" aligns with the PR objective of fixing the JS build system. This explicit configuration ensures proper module resolution when the package is used with AML in the CLI.
Let's verify the module system configuration across related files:
✅ Verification successful
The previous results showed ESM imports, but we need to verify the TypeScript configuration and if there's a mix of require/import statements to make a proper assessment about the module system configuration.
Module system configuration is correct and consistent
The configuration is properly set up for CommonJS:
- TypeScript is configured with
"module": "commonjs"in tsconfig.json - Package.json has
"type": "commonjs"explicitly set - The codebase uses TypeScript imports which will be transpiled to CommonJS during build
- No mixed usage of require/import was found
The setup is consistent with the PR's goal of fixing JS build to use AML in CLI, as it ensures proper module resolution at runtime.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent module system configuration across connector packages
# and verify no ESM-specific syntax is used in the codebase
# Check if all connector packages have consistent type configuration
echo "Checking package.json files in connector packages:"
fd package.json libs/connector- --exec jq -r '. | select(.type != null) | "\(.name): \(.type)"'
# Look for potential ESM syntax that might conflict with CommonJS
echo -e "\nChecking for ESM-specific syntax that might need attention:"
rg -t js -t ts "import\s+.*\s+from\s+" libs/connector-mariadb/
Length of output: 4158
Script:
#!/bin/bash
# Let's check the TypeScript configuration and build setup to ensure compatibility
echo "Checking tsconfig.json content:"
cat libs/connector-mariadb/tsconfig.json
echo -e "\nChecking build configuration in package.json:"
cat libs/connector-mariadb/package.json
echo -e "\nChecking for CommonJS require statements:"
rg "require\(" libs/connector-mariadb/src/
Length of output: 1968
cli/package.json (1)
Line range hint 13-13: Consider module system compatibility
This package uses "type": "module" while other packages in the PR are being changed to use "type": "commonjs". This inconsistency might lead to interoperability issues.
cli/src/convert.ts (1)
11-11: Verify AML module integration
The import of AML functions is added but they're not being used due to ESM compatibility issues (as noted in the FIXME comment below). Consider adding a TODO comment here to track this pending integration.
✅ Verification successful
Let me verify the usage of AML functions in the codebase.
Based on the search results, I can see that there's a FIXME comment in cli/src/convert.ts explaining that @azimutt/aml is using ESM which is causing compatibility issues. Let me verify the actual implementation to see if the functions are indeed commented out.
AML functions are currently disabled due to ESM compatibility issues
The import statement is correct, but the functions are intentionally disabled in the implementation due to ESM compatibility issues, as evidenced by the detailed FIXME comment. The code currently returns failure responses instead of using the imported functions:
parseDialect: returnsParserResult.failurewith "AML parser not available"generateDialect: returnsResult.failurewith "AML generator not available"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if @azimutt/aml is properly configured in package.json
cat package.json | jq '.dependencies["@azimutt/aml"]'
Length of output: 60
Script:
#!/bin/bash
# Check if the AML functions are used in the codebase
rg "generateAml|parseAml" -A 3
# Check package.json devDependencies for AML
cat package.json | jq '.devDependencies["@azimutt/aml"]'
# Look for any FIXME/TODO comments related to AML
rg -i "fixme.*aml|todo.*aml"
Length of output: 34897
Script:
#!/bin/bash
# Check the implementation in convert.ts
cat cli/src/convert.ts
Length of output: 4526
cli/src/index.ts (1)
85-85: LGTM: Debug option addition is consistent
The addition of the --debug option to the convert command follows the established pattern in other commands and will be particularly helpful for troubleshooting the AML integration issues.
| @@ -5,6 +5,7 @@ | |||
| "license": "MIT", | |||
| "homepage": "https://azimutt.app", | |||
| "keywords": [], | |||
| "type": "commonjs", | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent module system configuration across packages
The verification reveals mixed module system configurations:
- Most connector libraries use
"type": "commonjs" - Parser libraries (
parser-sql,parser-dbml,parser-prisma) use"type": "module" - CLI and AML packages use
"type": "module" - Some packages have no explicit type set
This inconsistency could lead to interoperability issues, especially since this PR aims to fix JS build for AML in CLI.
libs/utils/package.json: Setting tocommonjswhile dependent packages (CLI, AML) usemoduleneeds reviewlibs/parser-*/package.json: Using different module system than other libs needs justification- Root
package.json: Missing explicit type configuration
🔗 Analysis chain
LGTM! Verify consistency across packages.
The addition of "type": "commonjs" aligns with the PR objective of fixing the JS build system.
Let's verify consistency across related packages:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check type field consistency across all package.json files
# Expected: All related packages should have the same "type": "commonjs" configuration
# Find all package.json files and check their type field
fd package.json --type f --exec sh -c 'echo "=== $1 ==="; jq -r ".type // \"<not set>\"" "$1"' sh {}
Length of output: 1320
There was a problem hiding this comment.
That's indeed the main issue I try to solve here.
parser-sql, parser-dbml, parser-prisma and aml libs needs to be available as IIFE, I build them using Rollup that require ESM.
How to solve this?
There was a problem hiding this comment.
@loicknuchel, I understand the challenge you're facing.
Since parser-sql, parser-dbml, parser-prisma, and aml need to be built as IIFE bundles using Rollup (which requires ESM modules), but other parts of your codebase are using CommonJS ("type": "commonjs"), this inconsistency is causing interoperability issues.
To resolve this, you might consider the following approaches:
-
Standardize on ESM Modules Across Packages:
- Update all your packages to use ESM by setting
"type": "module"in theirpackage.jsonfiles. - This ensures consistency and leverages Rollup's ESM support.
- Example:
{ "type": "module" }
- Update all your packages to use ESM by setting
-
Dual Module Output:
- Configure your build process to output both ESM and CommonJS modules.
- Use tools like
tsupor configure Rollup to generate both formats. - Update the
package.jsonwith appropriateexports:{ "main": "./dist/index.cjs", "module": "./dist/index.mjs", "exports": { ".": { "import": "./dist/index.mjs", "require": "./dist/index.cjs" } } } - This allows compatibility with both ESM and CommonJS consumers.
-
Use Rollup Plugins for CommonJS:
- Incorporate the
@rollup/plugin-commonjsplugin in your Rollup configuration. - This allows Rollup to handle CommonJS modules within your ESM build.
- Rollup config example:
import commonjs from '@rollup/plugin-commonjs'; export default { input: 'src/index.js', output: { file: 'dist/bundle.js', format: 'iife', }, plugins: [ commonjs(), // other plugins ], };
- Incorporate the
-
Adjust Rollup Configuration to Handle Mixed Modules:
- Ensure Rollup is configured to resolve both ESM and CommonJS modules.
- Use the
@rollup/plugin-node-resolveplugin with themodule,main, andbrowserfields. - Example:
import resolve from '@rollup/plugin-node-resolve'; export default { // ... existing config plugins: [ resolve({ browser: true, preferBuiltins: false, }), // other plugins ], };
By standardizing your module system or configuring your build tools to handle both module types, you can resolve the inconsistencies and successfully build your libraries as IIFE bundles.
Let me know if you need further assistance or clarification on any of these steps.
| import * as vscode from "vscode"; | ||
| import { |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent vscode import styles detected across the extension
The codebase currently mixes two import styles for the vscode module:
- Namespace imports (
import * as vscode) - Direct type imports (
import { Type } from "vscode")
Files with mixed imports:
src/web/utils.tssrc/web/open.tssrc/web/extension.tssrc/web/convert.tssrc/web/rename.ts
For better maintainability and consistency, standardize on either namespace imports with type aliases or direct type imports throughout the extension.
🔗 Analysis chain
LGTM! Consider consolidating vscode imports
The change to namespace import is good practice. However, you could potentially consolidate the imports to either use the namespace for everything or move all needed types into the namespace import.
-import * as vscode from "vscode";
-import {
- ExtensionContext,
- TextDocument,
- TextDocumentChangeEvent,
- TextEditor,
- ViewColumn,
- WebviewPanel
-} from "vscode";
+import * as vscode from "vscode";
+type ExtensionContext = vscode.ExtensionContext;
+type TextDocument = vscode.TextDocument;
+type TextDocumentChangeEvent = vscode.TextDocumentChangeEvent;
+type TextEditor = vscode.TextEditor;
+type ViewColumn = vscode.ViewColumn;
+type WebviewPanel = vscode.WebviewPanel;Let's verify the consistency of vscode imports across the extension:
Let me check if there are any direct imports from 'vscode' to ensure complete consistency.
Let me fix the regex syntax and try again.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the consistency of vscode imports across the extension
# Expected: All files should use namespace imports consistently
echo "Checking for default imports:"
rg "^import vscode\b" "extensions/vscode-aml"
echo "Checking for namespace imports:"
rg "^import \* as vscode" "extensions/vscode-aml"
Length of output: 852
Script:
#!/bin/bash
# Check for any direct imports from 'vscode' module
rg "^import {[^}]+} from ['\"]vscode['\"]" "extensions/vscode-aml"
Length of output: 197
Script:
#!/bin/bash
# Check for any direct imports from 'vscode' module
rg "^import \{.*\} from ['\"]vscode['\"]" "extensions/vscode-aml"
Length of output: 572
| @@ -28,6 +28,7 @@ | |||
| "dry-publish": "pnpm run build && pnpm test && pnpm pack" | |||
| }, | |||
| "dependencies": { | |||
| "@azimutt/aml": "workspace:^", | |||
There was a problem hiding this comment.
💡 Codebase verification
Integration of @azimutt/aml is incomplete and currently non-functional
The dependency is imported but not properly integrated:
- There's a FIXME comment indicating ESM compatibility issues with the AML parser
- The AML parsing functionality is currently disabled, returning a "Not Implemented" error instead of using
parseAml - The code shows an unresolved module error when trying to use the AML package
🔗 Analysis chain
Verify AML dependency integration
The addition of @azimutt/aml aligns with the PR objective. However, ensure all CLI commands using this dependency are properly updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the new AML dependency is being used in the codebase
rg -l "@azimutt/aml" cli/src/
Length of output: 48
Script:
#!/bin/bash
# Check how @azimutt/aml is used in convert.ts
rg -A 5 "@azimutt/aml" cli/src/convert.ts
Length of output: 1556
| /* FIXME: @azimutt/aml is using ESM to be bundled with Rollup, but this produce and error here: | ||
| node:internal/modules/esm/resolve:257 | ||
| throw new ERR_MODULE_NOT_FOUND( | ||
| ^ | ||
| Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/.pnpm/@azimutt+aml@0.1.10/node_modules/@azimutt/aml/out/amlAst' imported from /node_modules/.pnpm/@azimutt+aml@0.1.10/node_modules/@azimutt/aml/out/index.js | ||
| at finalizeResolution (node:internal/modules/esm/resolve:257:11) | ||
| at moduleResolve (node:internal/modules/esm/resolve:913:10) | ||
| at defaultResolve (node:internal/modules/esm/resolve:1037:11) | ||
| at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:650:12) | ||
| at #cachedDefaultResolve (node:internal/modules/esm/loader:599:25) | ||
| at ModuleLoader.resolve (node:internal/modules/esm/loader:582:38) | ||
| at ModuleLoader.getModuleJobForImport (node:internal/modules/esm/loader:241:38) | ||
| at ModuleJob._link (node:internal/modules/esm/module_job:132:49) { | ||
| code: 'ERR_MODULE_NOT_FOUND', | ||
| url: 'file:///node_modules/.pnpm/@azimutt+aml@0.1.10/node_modules/@azimutt/aml/out/amlAst' | ||
| } | ||
| */ | ||
| if (dialect === 'aml') return ParserResult.failure([{message: 'AML parser not available', kind: 'NotImplemented', level: 'error', offset: {start: 0, end: 0}, position: {start: {line: 0, column: 0}, end: {line: 0, column: 0}}}]) // FIXME: return parseAml(content) |
There was a problem hiding this comment.
Critical: ESM compatibility issue blocks AML integration
The current ESM compatibility issue prevents the AML parser from working. This needs to be addressed before the AML integration can be completed.
Potential solutions to explore:
- Update the bundling configuration in @azimutt/aml to support both ESM and CommonJS
- Use dynamic imports for ESM modules
- Convert the CLI to use ESM exclusively
Would you like me to help create a GitHub issue to track this blocking issue?
Problem: can't use
@azimutt/amllib inazimuttCLI@azimutt/amllib is using ESM to be packaged a IIFE with RollupazimuttCLI is also ESMError:
Summary by CodeRabbit
Release Notes
New Features
--debugoption forconvert,diff, andclusteringcommands to enhance error visibility.@azimutt/amlfor improved AML parsing and generation capabilities.Bug Fixes
Documentation
"type": "commonjs".Chores