fix: en_7 recommendations... #1749
Conversation
# Conflicts: # packages/core/src/vcdm/hash/Keccak256.ts
WalkthroughThis pull request encompasses a comprehensive set of TypeScript configuration updates across multiple packages and applications. The changes primarily focus on updating module systems, compiler targets, and module resolution strategies to align with Node.js version 16 and ECMAScript 2023 standards. Additionally, several import statements have been modified to include explicit file extensions, particularly in the Vite integration project. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
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
Documentation and Community
|
Test CoverageSummary
|
lucanicoladebiasi
force-pushed
the
1662-en_7
branch
from
8fd2e7e to
81a99dd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/sdk-vite-integration/src/components/Hash.tsx (1)
3-3: Consider usingimport typefor type-only imports.Since
HashedContentis only used as a type, we can improve type safety by using theimport typesyntax.-import { HashedContent } from '../types/index.js'; +import type { HashedContent } from '../types/index.js';🧰 Tools
🪛 ESLint
[error] 3-3: All imports in the declaration are only used as types. Use
import type.(@typescript-eslint/consistent-type-imports)
apps/sdk-vite-integration/src/main.tsx (1)
4-4: Add semicolon for consistency.The static analysis tool suggests adding a semicolon after the import statement for consistent code formatting.
-import App from './App.js' +import App from './App.js';🧰 Tools
🪛 ESLint
[error] 4-4: Insert
;(prettier/prettier)
apps/sdk-vite-integration/tsconfig.app.json (1)
6-6: LGTM! Module configuration updates look good.The changes to use Node16 for both
moduleandmoduleResolutionare appropriate for modern Node.js applications. Note: The JSON comments in the file might cause issues with some tools - consider removing them if they're not essential.Also applies to: 10-10
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/sdk-cloudflare-integration/tsconfig.json(2 hunks)apps/sdk-nextjs-integration/tsconfig.json(2 hunks)apps/sdk-vite-integration/src/App.tsx(1 hunks)apps/sdk-vite-integration/src/components/GetLastBlock.tsx(1 hunks)apps/sdk-vite-integration/src/components/Hash.tsx(1 hunks)apps/sdk-vite-integration/src/components/TransferLogs.tsx(1 hunks)apps/sdk-vite-integration/src/const/index.tsx(1 hunks)apps/sdk-vite-integration/src/main.tsx(1 hunks)apps/sdk-vite-integration/src/types/index.tsx(1 hunks)apps/sdk-vite-integration/tests/Hash.spec.tsx(1 hunks)apps/sdk-vite-integration/tsconfig.app.json(1 hunks)apps/sdk-vite-integration/tsconfig.json(2 hunks)apps/sdk-vite-integration/tsconfig.node.json(1 hunks)docs/tsconfig.json(1 hunks)packages/aws-kms-adapter/tests/KMSVeChainSigner.solo.test.ts(3 hunks)packages/network/tests/signer/signers/vechain-private-key-signer/vechain-private-key-signer.unit.test.ts(7 hunks)packages/network/tests/thor-client/contracts/contract.event.solo.test.ts(4 hunks)packages/network/tests/thor-client/contracts/contract.solo.test.ts(1 hunks)packages/rpc-proxy/tsconfig.json(1 hunks)tsconfig.json(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- apps/sdk-vite-integration/src/const/index.tsx
- apps/sdk-vite-integration/src/App.tsx
- apps/sdk-vite-integration/src/components/GetLastBlock.tsx
- apps/sdk-vite-integration/src/types/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/sdk-vite-integration/tsconfig.node.json
[error] 8-8: JSON standard does not allow comments.
(parse)
[error] 9-9: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 9-9: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 9-9: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 9-9: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
apps/sdk-vite-integration/tsconfig.app.json
[error] 9-9: JSON standard does not allow comments.
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
🪛 ESLint
apps/sdk-vite-integration/src/components/Hash.tsx
[error] 3-3: All imports in the declaration are only used as types. Use import type.
(@typescript-eslint/consistent-type-imports)
apps/sdk-vite-integration/src/components/TransferLogs.tsx
[error] 7-7: All imports in the declaration are only used as types. Use import type.
(@typescript-eslint/consistent-type-imports)
[error] 7-7: Replace "../types/index.js" with '../types/index.js'
(prettier/prettier)
[error] 8-8: Replace "../const/index.js" with '../const/index.js'
(prettier/prettier)
apps/sdk-vite-integration/src/main.tsx
[error] 4-4: Insert ;
(prettier/prettier)
apps/sdk-vite-integration/tests/Hash.spec.tsx
[error] 3-3: Insert ;
(prettier/prettier)
[error] 6-6: Replace ··const·{·getByText·}·=·render(<Hash·/>) with ····const·{·getByText·}·=·render(<Hash·/>);
(prettier/prettier)
[error] 8-8: Replace ··await·expect.element(getByText('0xbf56c0728fd4e9cf64bfaf6dabab81554103298cdee5cc4d580433aa25e98b00')).toBeInTheDocument() with ····await·expect⏎········.element(⏎············getByText(⏎················'0xbf56c0728fd4e9cf64bfaf6dabab81554103298cdee5cc4d580433aa25e98b00'⏎············)⏎········)⏎········.toBeInTheDocument();
(prettier/prettier)
[error] 9-9: Insert ;
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit-integration-test-browser / Build & Lint (latest)
- GitHub Check: unit-integration-test-browser / Build & Lint (lts/*)
- GitHub Check: unit-integration-test-browser / Build & Lint (18)
- GitHub Check: unit-integration-test / Build & Lint (latest)
- GitHub Check: unit-integration-test / Build & Lint (lts/*)
- GitHub Check: unit-integration-test / Build & Lint (18)
🔇 Additional comments (14)
apps/sdk-vite-integration/tests/Hash.spec.tsx (1)
3-3: LGTM! Import path update aligns with module resolution requirements.The addition of the
.jsextension in the import statement is consistent with the PR's objective to improve module resolution.🧰 Tools
🪛 ESLint
[error] 3-3: Insert
;(prettier/prettier)
packages/network/tests/thor-client/contracts/contract.event.solo.test.ts (1)
104-104: LGTM! Consistent address formatting across test cases.The use of
Address.of()ensures proper address validation and formatting, which aligns with best practices for handling Ethereum addresses in tests.Also applies to: 479-479, 540-540
packages/network/tests/thor-client/contracts/contract.solo.test.ts (1)
657-657: LGTM! Consistent with address formatting in other tests.The use of
Address.of()here maintains consistency with the address formatting pattern used in other test files.packages/aws-kms-adapter/tests/KMSVeChainSigner.solo.test.ts (2)
9-14: LGTM! Clean import restructuring.The multi-line import structure improves readability and the addition of TypedDataDomain type enhances type safety.
305-308: Type safety improvement with TypedDataDomain.The explicit type casting of
typedData.domaintoTypedDataDomainenhances type safety and helps catch potential type-related issues at compile time.Also applies to: 315-317
packages/network/tests/signer/signers/vechain-private-key-signer/vechain-private-key-signer.unit.test.ts (2)
15-15: LGTM! Consistent type import.The addition of TypedDataDomain type import maintains consistency with other files.
297-300: Comprehensive type safety improvements.The consistent application of TypedDataDomain type casting across all test cases improves type safety and helps catch potential issues during testing. This includes:
- Invalid case testing
- Exception handling
- Ethers compatibility testing
- VeChain chainId variations
Also applies to: 321-324, 330-333, 352-355, 360-362, 382-387, 393-398
apps/sdk-vite-integration/tsconfig.json (1)
3-6: LGTM! TypeScript configuration updates look good.The changes to use ES2023 target and Node16 module system are appropriate and align with the PR objectives to specify concrete JavaScript versions.
Let's verify the Node.js version compatibility:
✅ Verification successful
✅ TypeScript configuration is compatible with the project setup
The ES2023 target and Node16 module system align perfectly with:
- TypeScript ~5.6.2 which fully supports these features
- Modern development stack (Vite 6, React 18)
- ESM package type configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Node.js version compatibility with ES2023 and verify package.json settings # Check if package.json specifies Node.js engine requirements rg '"engines":\s*{[^}]*"node":\s*"[^"]*"' package.json # Look for any TypeScript version specifications rg '"typescript":\s*"[^"]*"' package.jsonLength of output: 265
Script:
#!/bin/bash # Check package.json for Node.js engine requirements with fixed regex rg -U '"engines".*"node":\s*"[^"]*"' package.json # Check for any .nvmrc or .node-version files that might specify Node version fd -e nvmrc -e node-versionLength of output: 81
Script:
#!/bin/bash # Check if package.json exists and show its content if [ -f "apps/sdk-vite-integration/package.json" ]; then cat "apps/sdk-vite-integration/package.json" else # Check root package.json if project-specific one doesn't exist cat package.json fiLength of output: 1067
docs/tsconfig.json (1)
3-5: LGTM! Consistent TypeScript configuration for both compilation and ts-node.The changes maintain consistency between the main compiler options and ts-node settings, which is important for reliable behavior.
Also applies to: 11-13
packages/rpc-proxy/tsconfig.json (1)
7-8: LGTM! Package-specific TypeScript configuration is properly updated.The changes maintain consistency with the project-wide standards while preserving package-specific settings.
Also applies to: 12-12
apps/sdk-cloudflare-integration/tsconfig.json (1)
4-5: LGTM! Configuration updates are compatible with Cloudflare Workers.The TypeScript configuration changes align with project standards while maintaining Cloudflare Workers compatibility through appropriate type definitions.
Let's verify Cloudflare Workers compatibility:
Also applies to: 9-9
✅ Verification successful
✅ Configuration verified as compatible with Cloudflare Workers
The
nodejs_compatflag in wrangler.toml explicitly enables Node.js compatibility mode, confirming that the Node16 module system and ES2023 target are fully supported in the Workers runtime.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Cloudflare Workers compatibility with Node16 module system # Check Cloudflare Workers types version rg '"@cloudflare/workers-types":\s*"[^"]*"' package.json # Look for any wrangler configuration that might affect module compatibility fd -t f wrangler.toml -x cat {}Length of output: 228
apps/sdk-vite-integration/tsconfig.node.json (1)
5-5: LGTM! Node-specific TypeScript configuration for Vite is properly updated.The changes maintain compatibility with Vite while aligning with the project's Node16 module system standards.
Let's verify Vite compatibility:
Also applies to: 9-9
✅ Verification successful
✅ Verified: Node16 module system is fully compatible with the Vite setup
The Vite configuration follows standard practices and doesn't contain any module resolution customizations that could conflict with the Node16 module system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Vite compatibility with Node16 module system # Check Vite version rg '"vite":\s*"[^"]*"' package.json # Look for any Vite configuration that might affect module resolution fd -t f 'vite.config.ts' -x cat {}Length of output: 234
tsconfig.json (1)
8-9: Consider aligning ES target versions across configurations.While the Node16 module configuration is consistent, there's a discrepancy in the ES target versions:
- Root config uses ES2023
- Vite app config uses ES2020
Consider aligning these for consistency, unless there's a specific reason for the difference.
Let's check for other ES target configurations in the project:
Also applies to: 13-13, 25-25
✅ Verification successful
ES target differences appear to be intentionally environment-specific
The varying ES targets across the project actually follow a logical pattern:
- Browser-focused integrations (NextJS, Hardhat) use ES2020 for broader compatibility
- Node.js-focused components use ES2023 to leverage newer features
These differences appear to be a deliberate choice based on runtime requirements rather than an inconsistency that needs to be addressed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for target configurations in all tsconfig files fd tsconfig.json | xargs grep -l "target"Length of output: 278
Script:
#!/bin/bash # Extract target configurations from all tsconfig files echo "=== Checking ES target configurations ===" fd tsconfig.json | while read -r file; do echo "File: $file" grep -A 1 '"target"' "$file" || grep -A 1 "'target'" "$file" || true echo "---" doneLength of output: 3781
apps/sdk-nextjs-integration/tsconfig.json (1)
11-12: LGTM! Configuration updates are consistent.The changes to use Node16 for both
moduleandmoduleResolutionalign well with the other configurations and modern Node.js practices. The ES2020 target is consistent with the Vite integration configuration.Also applies to: 33-33
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/sdk-vite-integration/src/components/TransferLogs.tsx(1 hunks)apps/sdk-vite-integration/src/main.tsx(1 hunks)apps/sdk-vite-integration/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/sdk-vite-integration/tsconfig.json
🧰 Additional context used
🪛 ESLint
apps/sdk-vite-integration/src/components/TransferLogs.tsx
[error] 7-7: All imports in the declaration are only used as types. Use import type.
(@typescript-eslint/consistent-type-imports)
[error] 7-7: Replace "../types/index.js" with '../types/index.js'
(prettier/prettier)
[error] 8-8: Replace "../const/index.tsx" with '../const/index.tsx'
(prettier/prettier)
apps/sdk-vite-integration/src/main.tsx
[error] 4-4: Insert ;
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: rpc-proxy / test / test
- GitHub Check: unit-integration-test-browser / Build & Lint (latest)
- GitHub Check: unit-integration-test / Build & Lint (latest)
- GitHub Check: unit-integration-test-browser / Build & Lint (lts/*)
- GitHub Check: unit-integration-test / Build & Lint (lts/*)
- GitHub Check: unit-integration-test-browser / Build & Lint (18)
- GitHub Check: unit-integration-test / Build & Lint (18)
- GitHub Check: install-build / Build & Lint
- GitHub Check: test-apps / Install and test example apps
- GitHub Check: Execute doc examples
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/sdk-vite-integration/src/main.tsx (1)
4-4: LGTM! Verify Vite configuration for TypeScript extensions.The explicit
.tsxextension aligns with Node16 module resolution standards and the PR objectives. However, ensure that your Vite configuration properly handles TypeScript extensions.#!/bin/bash # Description: Verify Vite handles TypeScript extensions correctly # Check Vite configuration echo "Checking Vite resolve.extensions configuration..." rg -A 5 'resolve.*extensions' apps/sdk-vite-integration/vite.config.* # Check if TypeScript is configured for Node16 resolution echo "Checking TypeScript module resolution..." rg '"moduleResolution"' apps/sdk-vite-integration/tsconfig.*🧰 Tools
🪛 ESLint
[error] 4-4: Insert
;(prettier/prettier)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/sdk-vite-integration/src/components/TransferLogs.tsx (1)
7-8: 🛠️ Refactor suggestionOptimize imports and maintain consistent style.
The import statements need the following improvements:
- Use
import typefor theTransfertype since it's only used as a type annotation- Use single quotes for consistency with the project style
Apply this diff to optimize the imports:
-import { Transfer } from "../types/index.tsx"; -import { explorerUrl, thorClient } from "../const/index.tsx"; +import type { Transfer } from '../types/index.tsx'; +import { explorerUrl, thorClient } from '../const/index.tsx';🧰 Tools
🪛 ESLint
[error] 7-7: All imports in the declaration are only used as types. Use
import type.(@typescript-eslint/consistent-type-imports)
[error] 7-7: Replace
"../types/index.tsx"with'../types/index.tsx'(prettier/prettier)
[error] 8-8: Replace
"../const/index.tsx"with'../const/index.tsx'(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/sdk-vite-integration/src/components/TransferLogs.tsx(1 hunks)
🧰 Additional context used
🪛 ESLint
apps/sdk-vite-integration/src/components/TransferLogs.tsx
[error] 7-7: All imports in the declaration are only used as types. Use import type.
(@typescript-eslint/consistent-type-imports)
[error] 7-7: Replace "../types/index.tsx" with '../types/index.tsx'
(prettier/prettier)
[error] 8-8: Replace "../const/index.tsx" with '../const/index.tsx'
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: rpc-proxy / test / test
- GitHub Check: unit-integration-test-browser / Build & Lint (latest)
- GitHub Check: unit-integration-test-browser / Build & Lint (lts/*)
- GitHub Check: unit-integration-test-browser / Build & Lint (18)
- GitHub Check: test-apps / Install and test example apps
- GitHub Check: unit-integration-test / Build & Lint (latest)
- GitHub Check: unit-integration-test / Build & Lint (lts/*)
- GitHub Check: unit-integration-test / Build & Lint (18)
- GitHub Check: install-build / Build & Lint
- GitHub Check: Execute doc examples
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/sdk-vite-integration/src/components/Hash.tsx (1)
3-3: Useimport typefor type-only imports.Since
HashedContentis only used as a type, we should use theimport typesyntax to make this explicit and help with bundle optimization.Apply this change:
-import { HashedContent } from '../types/index.tsx'; +import type { HashedContent } from '../types/index.tsx';🧰 Tools
🪛 ESLint
[error] 3-3: All imports in the declaration are only used as types. Use
import type.(@typescript-eslint/consistent-type-imports)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/sdk-vite-integration/src/App.tsx(1 hunks)apps/sdk-vite-integration/src/components/GetLastBlock.tsx(1 hunks)apps/sdk-vite-integration/src/components/Hash.tsx(1 hunks)apps/sdk-vite-integration/src/const/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/sdk-vite-integration/src/components/GetLastBlock.tsx
- apps/sdk-vite-integration/src/const/index.tsx
- apps/sdk-vite-integration/src/App.tsx
🧰 Additional context used
🪛 ESLint
apps/sdk-vite-integration/src/components/Hash.tsx
[error] 3-3: All imports in the declaration are only used as types. Use import type.
(@typescript-eslint/consistent-type-imports)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: rpc-proxy / test / test
- GitHub Check: unit-integration-test / Build & Lint (latest)
- GitHub Check: unit-integration-test-browser / Build & Lint (latest)
- GitHub Check: unit-integration-test / Build & Lint (lts/*)
- GitHub Check: unit-integration-test-browser / Build & Lint (lts/*)
- GitHub Check: unit-integration-test / Build & Lint (18)
- GitHub Check: unit-integration-test-browser / Build & Lint (18)
- GitHub Check: test-apps / Install and test example apps
- GitHub Check: install-build / Build & Lint
- GitHub Check: Execute doc examples
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/sdk-vite-integration/src/components/Hash.tsx (1)
3-3: LGTM! The explicit file extension improves module resolution.The updated import path with explicit
.tsxextension aligns with modern TypeScript best practices and improves module resolution clarity.Let's verify that this change is consistent across the codebase:
✅ Verification successful
The explicit file extension in import path is verified and consistent with the codebase structure.
The import path update to include
.tsxextension is correct and isolated to this file. The types directory exists at the expected location and no other files need similar updates.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports from '../types' without file extension # to ensure consistency across the codebase rg "from '\.\./types';" --type ts --type tsxLength of output: 80
Script:
#!/bin/bash # First, let's find all files in the types directory echo "=== Files in types directory ===" fd . -e ts -e tsx --full-path "**/types/" echo -e "\n=== Checking import statements from types (with various extensions) ===" # Use -t typescript to handle both .ts and .tsx files rg "from ['\"].*types.*['\"]" -t typescript echo -e "\n=== Using ast-grep to find import statements ===" ast-grep --pattern 'import { $_ } from "../types$_"'Length of output: 627
Script:
#!/bin/bash # Find all TypeScript/React files echo "=== TypeScript/React Files ===" fd . -e ts -e tsx echo -e "\n=== Checking type imports with correct flags ===" rg "from.*types" --type-add 'ts:*.{ts,tsx}' -t ts echo -e "\n=== Directory structure around the file ===" fd . --full-path "**/sdk-vite-integration/src/"Length of output: 66517
🧰 Tools
🪛 ESLint
[error] 3-3: All imports in the declaration are only used as types. Use
import type.(@typescript-eslint/consistent-type-imports)
Description
EN_7 recommendations.
Specify target JS.
Fixes # EN_7
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
yarn examplesyarn test:soloyarn test:unitTest Configuration:
Summary by CodeRabbit
TypeScript Configuration Updates
.tsextensionsImport and Export Path Updates
.jsfile extensions to import statements in Vite integrationTesting Improvements