cli: refactor build, dev, start commands #111
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
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
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: 23
🧹 Outside diff range and nitpick comments (22)
packages/cli/runtime-meta.d.ts (1)
1-2: Add JSDoc comments to document the constants' purpose.While the constant names are descriptive, adding JSDoc comments would improve documentation and provide better IDE support.
+/** The root directory of the package */ export declare const pkgDir: string; +/** The directory containing runtime-related files */ export declare const runtimeDir: string;packages/cli/src/config/index.ts (1)
4-8: LGTM! Improved type safety with stricter generic defaults.The change from
{}toRecord<string, never>as default generic parameters is a good improvement. This makes the type system more strict and prevents accidental property assignments in empty configurations.This change aligns well with TypeScript best practices and provides better type safety. The stricter typing will help catch potential configuration errors at compile time rather than runtime.
packages/cli/src/types/hooks.ts (1)
7-7: LGTM, but documentation needed for restart hooks.While the addition of the "dev:restart" hook is appropriate, there are multiple restart-related hooks ("dev:restart", "dev:reload", "rollup:reload", "restart") that could benefit from documentation explaining their distinct purposes and execution order.
Consider adding JSDoc comments to clarify:
+ /** Triggered when the dev process needs to restart (e.g., when Rollup watcher starts/restarts) */ "dev:restart": () => void; + /** Triggered when files need to be reloaded during development */ "dev:reload": () => void; + /** Triggered when Rollup needs to reload configuration */ "rollup:reload": () => void; + /** Triggered for a full restart of the application */ restart: () => void;packages/cli/src/core/config/defaults.ts (1)
5-5: Consider adding JSDoc comment for the new config option.Adding documentation for the
indexersDirconfiguration would help users understand its purpose and relationship with the build system.export const ApibaraDefaults: ApibaraConfig = { rootDir: ".", + /** Directory containing indexer definitions, used during build time to generate the indexer list. */ indexersDir: "indexers",packages/cli/src/core/build/prepare.ts (1)
Line range hint
5-16: Consider enhancing error handling for directory operations.While the current implementation relies on Promise rejections, consider adding explicit error handling for directory operations to provide more informative error messages to users.
Here's a suggested improvement:
export async function prepare(apibara: Apibara) { - await prepareDir(apibara.options.buildDir); - await prepareDir(apibara.options.outputDir); + try { + await prepareDir(apibara.options.buildDir); + await prepareDir(apibara.options.outputDir); + apibara.logger.success("Output directory cleaned"); + } catch (error) { + apibara.logger.error(`Failed to prepare directories: ${error.message}`); + throw error; + } - apibara.logger.success("Output directory cleaned"); } async function prepareDir(dir: string) { - await fsp.mkdir(dir, { recursive: true }); - await fse.emptyDir(dir); + try { + await fsp.mkdir(dir, { recursive: true }); + await fse.emptyDir(dir); + } catch (error) { + throw new Error(`Failed to prepare directory ${dir}: ${error.message}`); + } }packages/cli/src/rollup/plugins/indexers.ts (1)
4-5: Add input validation and document the deduplication purpose.While the deduplication logic is correct, consider adding:
- Input validation for
apibara.indexers- Documentation explaining why deduplication is necessary
Consider this improvement:
export function indexers(apibara: Apibara) { + if (!Array.isArray(apibara.indexers)) { + throw new Error('apibara.indexers must be an array'); + } + // Deduplicate indexers to prevent multiple imports of the same module const indexers = [...new Set(apibara.indexers)];packages/cli/src/types/apibara.ts (1)
6-11: Enhance type definition documentation and constraints.While the type definition is clear, consider these improvements:
- Add JSDoc comments with more detailed descriptions
- Consider using a more specific type for the
indexerpath propertyHere's a suggested improvement:
+/** + * Represents an indexer configuration within the Apibara system. + */ export type IndexerDefinition = { + /** + * Unique identifier for the indexer. + * This name is used to reference the indexer in CLI commands. + */ name: string; + /** + * Relative or absolute path to the indexer implementation file. + * The file should export the indexer configuration. + */ indexer: string; };packages/cli/src/core/build/prod.ts (1)
Line range hint
18-18: Enhance error message for missing output configuration.Consider providing more context in the error message to help users troubleshoot configuration issues.
- throw new Error("No output options specified in Rollup config"); + throw new Error("No output options specified in Rollup config. Please define either a single output option or an array of outputs in your configuration.");packages/cli/src/core/apibara.ts (1)
24-24: Consider type safety for indexers arrayWhile initializing
indexersas an empty array is correct, consider explicitly typing it for better type safety and documentation.- indexers: [], + indexers: Array<IndexerDefinition> = [],Please ensure
IndexerDefinitiontype is imported from the appropriate module.packages/cli/build.config.ts (1)
32-38: Consider documenting the runtime-meta.mjs purposeThe runtime-meta.mjs file is referenced outside the src directory, which is an unusual pattern. Consider:
- Adding a comment explaining its purpose and relationship to the runtime module
- Documenting why it needs to be external to the src directory
packages/cli/src/core/scan.ts (1)
5-5: Consider making the constant immutable.To prevent accidental modifications, consider making the array readonly.
-const INDEXER_EXTENSIONS = [".indexer.ts", ".indexer.js"]; +const INDEXER_EXTENSIONS = Object.freeze([".indexer.ts", ".indexer.js"]) as readonly string[];packages/cli/src/runtime/start.ts (2)
36-44: Consider aligning CLI name with project branding.The name "indexer-runner" might not align with the Apibara branding. Consider using "apibara" or a similar brand-aligned name.
export const mainCli = defineCommand({ meta: { - name: "indexer-runner", + name: "apibara", description: "Run an indexer", }, subCommands: { start: () => startCommand, }, });
46-48: Remove unnecessary default export.The empty default export serves no purpose and can be removed.
runMain(mainCli); - -export default {};packages/cli/src/cli/commands/start.ts (2)
20-23: Consider adding validation for the preset argument.The preset argument accepts any string value without validation. Consider adding allowed values or a validation function to ensure only supported presets are used.
preset: { type: "string", description: "The preset to use", + validate: (value: string) => ["preset1", "preset2"].includes(value), + options: ["preset1", "preset2"], },
43-43: Consider using unique build directories for concurrent runs.Using a fixed build directory (
.apibara/build) could cause issues if multiple instances of the CLI are running simultaneously.Consider:
- Using a unique directory per run (e.g., including a timestamp or UUID)
- Implementing a cleanup mechanism for old builds
- Adding a lock mechanism to prevent concurrent builds
packages/cli/src/core/config/loader.ts (2)
Line range hint
28-29: Address the linting issue instead of ignoring it.The code uses
biome-ignore lint: noParameterAssignto suppress a linting warning about parameter reassignment. Consider refactoring to avoid parameter mutation.- // biome-ignore lint: noParameterAssign - configOverrides = klona(configOverrides); + const clonedOverrides = klona(configOverrides);Then update subsequent uses of
configOverridesto useclonedOverrides.
Line range hint
28-54: Consider splitting configuration loading and processing.The
_loadUserConfigfunction handles multiple responsibilities:
- Configuration loading
- Deep cloning
- Metadata attachment
- Dev mode handling
This could make the function harder to maintain as more features are added.
Consider splitting this into smaller, focused functions:
async function loadRawConfig(overrides: ApibaraConfig, opts: LoadConfigOptions) { return (opts.watch ? watchConfig : loadConfig)({...}); } function processLoadedConfig( loadedConfig: LoadResult<ApibaraConfig>, overrides: ApibaraConfig, dev: boolean ): ApibaraOptions { const options = klona(loadedConfig.config); options._config = overrides; options._c12 = loadedConfig; options.dev = dev; return options; }This would make the code more maintainable and easier to test.
packages/cli/src/types/config.ts (1)
63-64: Add JSDoc comment for the indexersDir propertyWhile the addition of
indexersDiraligns with the PR's objectives, adding a JSDoc comment would help users understand its purpose and expected value.+ /** Directory containing the indexer definitions */ indexersDir: string;packages/cli/src/cli/commands/dev.ts (1)
Line range hint
101-116: Consider enhancing error handling for child process.While the process exit code is logged, there's no handling of process spawn errors or unexpected terminations.
Consider adding error handling:
childProcess = spawn("node", childArgs, { stdio: "inherit", }); +childProcess.on("error", (error) => { + apibara.logger.error(`Failed to start indexers: ${error.message}`); +}); + childProcess.on("close", (code) => { if (code !== null) { - apibara.logger.log(`Indexers process exited with code ${code}`); + if (code === 0) { + apibara.logger.success("Indexers process completed successfully"); + } else { + apibara.logger.error(`Indexers process failed with code ${code}`); + } } });packages/cli/src/core/build/types.ts (2)
Line range hint
11-83: Add error handling for type generation operations.The type generation logic lacks proper error handling for file operations and type generation. Consider adding try-catch blocks and error logging to handle potential failures gracefully.
Here's a suggested improvement for the file operations:
- await Promise.all( - buildFiles.map(async (file) => { - const _file = resolve(apibara.options.buildDir, file.path); - await fsp.mkdir(dirname(_file), { recursive: true }); - await fsp.writeFile(_file, file.contents); - }), - ); + try { + await Promise.all( + buildFiles.map(async (file) => { + const _file = resolve(apibara.options.buildDir, file.path); + try { + await fsp.mkdir(dirname(_file), { recursive: true }); + await fsp.writeFile(_file, file.contents); + } catch (err) { + apibara.logger.error(`Failed to write file ${file.path}: ${err.message}`); + throw err; + } + }), + ); + } catch (err) { + apibara.logger.error('Type generation failed'); + throw err; + }
Line range hint
84-133: Consider refactoring path handling logic for better maintainability.The path handling logic, particularly for TSConfig generation, is complex and could benefit from:
- Moving the
relativeWithDotutility to a separate utilities file- Creating dedicated functions for path normalization
- Adding documentation for the path manipulation logic
Consider creating a new utility file
packages/cli/src/core/utils/path.ts:/** * Ensures a path starts with './' if it's not already a relative path * @param from - The base directory path * @param to - The target path to make relative * @returns A properly formatted relative path */ export function relativeWithDot(from: string, to: string) { const rel = relative(from, to); return RELATIVE_RE.test(rel) ? rel : `./${rel}`; } /** * Normalizes TSConfig paths by converting absolute paths to relative * @param tsconfigDir - The directory containing tsconfig * @param paths - Array of paths to normalize */ export async function normalizeTSConfigPaths(tsconfigDir: string, paths: string[]) { return Promise.all( paths.map(async (path) => { if (!isAbsolute(path)) { return path; } const stats = await fsp.stat(path).catch(() => null); return relativeWithDot( tsconfigDir, stats?.isFile() ? path.replace(/(?<=\w)\.\w+$/g, "") : path, ); }), ); }Also applies to: 136-141
packages/cli/src/runtime/dev.ts (1)
64-64: Consider removing unnecessary default exportThe default export of an empty object may not be necessary if it is not used elsewhere.
You can remove the default export if it's not needed:
- export default {};
📜 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 (40)
change/apibara-ed31dbbf-efbd-44c6-8eb0-f729dde0acb6.json(1 hunks)packages/cli/build.config.ts(2 hunks)packages/cli/package.json(4 hunks)packages/cli/playground/apibara.config.ts(1 hunks)packages/cli/playground/indexers/starknet.indexer.ts(2 hunks)packages/cli/playground/tsconfig.json(0 hunks)packages/cli/runtime-meta.d.ts(1 hunks)packages/cli/runtime-meta.mjs(1 hunks)packages/cli/src/cli/commands/build.ts(2 hunks)packages/cli/src/cli/commands/dev.ts(5 hunks)packages/cli/src/cli/commands/prepare.ts(0 hunks)packages/cli/src/cli/commands/start.ts(1 hunks)packages/cli/src/cli/index.ts(1 hunks)packages/cli/src/config/index.ts(1 hunks)packages/cli/src/core/apibara.ts(3 hunks)packages/cli/src/core/build/build.ts(1 hunks)packages/cli/src/core/build/dev.ts(1 hunks)packages/cli/src/core/build/prepare.ts(1 hunks)packages/cli/src/core/build/prod.ts(2 hunks)packages/cli/src/core/build/types.ts(2 hunks)packages/cli/src/core/config/defaults.ts(1 hunks)packages/cli/src/core/config/loader.ts(1 hunks)packages/cli/src/core/config/resolvers/runtime-config.resolver.ts(1 hunks)packages/cli/src/core/config/update.ts(1 hunks)packages/cli/src/core/scan.ts(1 hunks)packages/cli/src/internal/citty/index.ts(0 hunks)packages/cli/src/internal/consola/index.ts(0 hunks)packages/cli/src/rollup/config.ts(3 hunks)packages/cli/src/rollup/plugins/config.ts(1 hunks)packages/cli/src/rollup/plugins/indexers.ts(1 hunks)packages/cli/src/runtime/dev.ts(1 hunks)packages/cli/src/runtime/index.ts(1 hunks)packages/cli/src/runtime/internal/app.ts(1 hunks)packages/cli/src/runtime/start.ts(1 hunks)packages/cli/src/types/apibara.ts(1 hunks)packages/cli/src/types/config.ts(3 hunks)packages/cli/src/types/hooks.ts(1 hunks)packages/cli/src/types/virtual/config.d.ts(1 hunks)packages/cli/src/types/virtual/indexers.d.ts(1 hunks)packages/cli/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (4)
- packages/cli/playground/tsconfig.json
- packages/cli/src/cli/commands/prepare.ts
- packages/cli/src/internal/citty/index.ts
- packages/cli/src/internal/consola/index.ts
✅ Files skipped from review due to trivial changes (3)
- change/apibara-ed31dbbf-efbd-44c6-8eb0-f729dde0acb6.json
- packages/cli/src/runtime/index.ts
- packages/cli/src/types/virtual/config.d.ts
🔇 Additional comments (51)
packages/cli/runtime-meta.d.ts (1)
1-2: Verify consistent path resolution across the codebase.
The declarations look good. Let's verify that these constants are used consistently for path resolution.
✅ Verification successful
Based on the results, I can now generate the final response:
Path resolution is consistent across the codebase
The verification shows that:
runtimeDiris consistently used withjoinfrom thepathepackage for resolving runtime-related paths in CLI commandspkgDiris properly defined usingfileURLToPathfor package-level path resolution- Other path resolution methods (
path.join,path.resolve) are used in different packages for different purposes, not conflicting with these constants
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of pkgDir and runtimeDir constants
# Expected: These constants should be used for path resolution and not mixed with other path resolution methods
# Check how these constants are used in path resolution
echo "Checking usage of pkgDir and runtimeDir..."
rg -A 3 "pkgDir|runtimeDir"
# Check for potential inconsistent path resolution methods
echo "Checking for other path resolution methods..."
ast-grep --pattern 'path.join($$$)'
ast-grep --pattern 'path.resolve($$$)'
Length of output: 3063
packages/cli/runtime-meta.mjs (3)
1-1: LGTM! Using the recommended import for URL handling.
The use of node:url protocol ensures consistent resolution of the built-in module across different Node.js versions.
3-3: LGTM! Correct implementation of package directory resolution.
The implementation correctly resolves the package directory using fileURLToPath and import.meta.url, which is the recommended approach for ES modules.
5-7: Verify runtime directory existence during build.
While the path resolution is implemented correctly, we should ensure the dist/runtime/ directory exists when these paths are used.
Let's verify the directory structure:
Consider adding a build step to ensure this directory exists:
+// Add to your build script
+import { mkdir } from 'node:fs/promises';
+await mkdir(runtimeDir, { recursive: true });packages/cli/src/rollup/plugins/config.ts (2)
1-2: LGTM! Imports are correctly structured.
The imports are well-organized, using proper type imports for better tree-shaking.
4-10: 🛠️ Refactor suggestion
Verify sensitive data handling and add validation.
The function stringifies the entire options object without filtering sensitive data or validating its structure.
Let's check for potential sensitive data in the options:
Consider these improvements:
- Add options validation
- Filter out sensitive data before stringifying
- Add type safety for the options structure
Here's a suggested implementation:
import virtual from "@rollup/plugin-virtual";
import type { Apibara } from "apibara/types";
function sanitizeOptions(options: unknown) {
// Add validation and sanitization logic
if (!options || typeof options !== 'object') {
throw new Error('Invalid options');
}
// Deep clone and remove sensitive fields
return JSON.parse(JSON.stringify(options, (_, value) =>
// Example: mask sensitive field values
typeof value === 'string' && /password|secret|key|token/i.test(_)
? '[REDACTED]'
: value
));
}
export function appConfig(apibara: Apibara) {
const safeOptions = sanitizeOptions(apibara.options);
return virtual({
"#apibara-internal-virtual/config": `
export const config = ${JSON.stringify(safeOptions, null,
process.env.NODE_ENV === 'production' ? 0 : 2)};
`,
});
}packages/cli/playground/apibara.config.ts (1)
3-13: LGTM! Configuration structure is clean and well-organized.
The configuration structure with separate sections for base config and presets provides good flexibility for different environments.
packages/cli/src/core/config/defaults.ts (1)
5-5: Verify compatibility of default indexers directory.
The addition of indexersDir aligns well with the refactoring goals. However, let's verify that the default "indexers" path works correctly with the existing project structure.
✅ Verification successful
Let me gather more specific information about the actual indexer file locations and usage patterns.
Let me try a different approach to find indexer file patterns and their usage.
Based on the results, I can now generate the final response:
Default indexers directory is compatible with existing structure
The default indexersDir: "indexers" aligns perfectly with the current codebase organization, as evidenced by:
- Example indexer files in
examples/cli/indexers/ - Playground indexers in
packages/cli/playground/indexers/
These existing implementations already follow the proposed default directory structure, confirming its compatibility with the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any existing indexer files are already using a different directory structure
fd -t f -e ts -e js --full-path indexer
Length of output: 1967
Script:
#!/bin/bash
# Check for actual indexer file patterns and their locations
rg -l "\.indexer\.(ts|js)$"
# Look for any imports or references to indexer paths in CLI code
rg -g '*.ts' -g '*.js' "indexers.*\.(ts|js)" packages/cli/
Length of output: 90
Script:
#!/bin/bash
# Find indexer files in examples and playground directories
fd -t f ".*indexer\.(ts|js)$"
# Look for code that loads or processes indexer files
ast-grep --pattern 'from "$_/indexers/$_"'
ast-grep --pattern 'require("$_/indexers/$_")'
Length of output: 365
packages/cli/src/core/build/prepare.ts (1)
9-9: LGTM! Consistent with centralized logging approach.
The change aligns with the PR's objective of centralizing logging through apibara.logger.
Let's verify the logger interface implementation:
✅ Verification successful
Based on the extensive search results, I can now generate the final response:
Logger implementation verified and consistent across codebase
The apibara.logger usage is consistent throughout the codebase and properly implemented:
- Defined in core/apibara.ts as part of the Apibara interface with consola integration
- Used consistently for logging with standard methods (success, info, warn, error)
- Follows unified logging pattern across all CLI operations and build processes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the apibara logger interface and its usage across the codebase
# Expected: Find the logger interface definition and consistent usage pattern
# Search for logger interface definition
echo "Searching for logger interface definition..."
ast-grep --pattern 'interface $_ {
$$$
success($_): $_
$$$
}'
# Search for logger usage patterns
echo "Searching for logger usage patterns..."
rg "logger\.(success|info|warn|error)"
Length of output: 1275
Script:
#!/bin/bash
# Let's verify the logger type definition and its usage pattern
# by checking the imports and type definitions
# Search for apibara type imports and logger type definitions
echo "Searching for apibara type imports..."
rg "from ['\"]@apibara/types['\"]" -A 3
# Search for logger type declarations
echo "Searching for logger type declarations..."
ast-grep --pattern 'type $_ = {
$$$
logger: $_
$$$
}'
# Search for any potential logger interface declarations
echo "Searching for logger interfaces..."
ast-grep --pattern 'interface Logger {
$$$
}'
Length of output: 414
Script:
#!/bin/bash
# Let's try a different approach to find logger-related types and interfaces
# by searching for more generic patterns and examining the apibara context
# Search for any interface or type with logger-related methods
echo "Searching for logger-related interfaces..."
ast-grep --pattern 'interface $_ {
success($_): void;
}'
# Look for apibara context type definitions
echo "Searching for apibara context..."
rg "apibara" --type ts -B 2 -A 2
# Check the actual usage context
echo "Checking logger usage context..."
rg "import.*apibara" --type ts -A 3
Length of output: 69277
packages/cli/src/types/virtual/indexers.d.ts (2)
10-10: LGTM! Verify virtual module integration.
The empty array is correctly typed and will serve as a placeholder for Rollup's virtual plugin to populate at build time.
#!/bin/bash
# Description: Verify the virtual module integration with Rollup
# Look for Rollup configuration that handles this virtual module
echo "Checking Rollup configuration for virtual module handling:"
rg -l "indexers.*virtual" packages/cli
# Look for code that consumes this virtual module
echo -e "\nChecking usage of the virtual module:"
rg -l "from ['\"].*virtual/indexers['\"]" packages/cli/src1-2: Verify import path resolution.
The import paths look correct, but let's verify that they resolve properly, especially the local import from "apibara/types".
✅ Verification successful
Import paths are correctly configured and used
The verification shows that:
@apibara/indexeris properly declared as a workspace dependencyApibaraRuntimeConfigis defined inpackages/cli/src/types/runtime.tsand is correctly imported by other files- Both types (
IndexerWithStreamConfigandApibaraRuntimeConfig) are actively used in the codebase - The import paths align with the project's structure and TypeScript module resolution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the imported types exist in the specified locations
# Check if the @apibara/indexer package is properly declared in package.json
echo "Checking @apibara/indexer in package.json:"
jq '.dependencies["@apibara/indexer"] // .devDependencies["@apibara/indexer"]' packages/cli/package.json
# Look for the local types file
echo -e "\nLocating apibara/types definition:"
fd -e ts -e d.ts "apibara.ts|types.ts|types.d.ts" packages/cli/src/types
Length of output: 546
Script:
#!/bin/bash
# Let's verify the imports more thoroughly
# Check package.json for dependencies
echo "Checking package.json for @apibara/indexer:"
cat packages/cli/package.json | jq '.dependencies["@apibara/indexer"] // .devDependencies["@apibara/indexer"]'
# Search for apibara types definition
echo -e "\nSearching for apibara types definition:"
fd "types.ts" "types.d.ts" packages/cli/src
# Check if IndexerWithStreamConfig type is used elsewhere
echo -e "\nChecking usage of IndexerWithStreamConfig:"
rg "IndexerWithStreamConfig" packages/cli/src
# Check if ApibaraRuntimeConfig type is used elsewhere
echo -e "\nChecking usage of ApibaraRuntimeConfig:"
rg "ApibaraRuntimeConfig" packages/cli/src
# Look for any potential type definition files in the project
echo -e "\nLooking for type definitions:"
fd -e d.ts . packages/cli/src/types
Length of output: 1835
packages/cli/src/core/build/build.ts (2)
8-8: LGTM! Good code structure.
The added blank lines improve readability by clearly separating the configuration, hook execution, and build logic sections.
Also applies to: 10-10
9-9: LGTM! Verify hook implementation across consumers.
The hook implementation is well-structured and follows common patterns. The "rollup:before" hook provides a good extension point for pre-build customizations.
Let's verify the hook usage across the codebase:
✅ Verification successful
Let me gather more information about the hook implementation and its usage.
Let me check the hook system implementation in the Apibara class to understand how hooks are managed.
Based on the gathered information, I can now provide the final response:
Hook implementation is correctly integrated and safe to use
The "rollup:before" hook is:
- Properly defined in the type system (
ApibaraHooksinterface) - Correctly initialized using the
hookablelibrary in the Apibara class - Used appropriately in the build process with proper typing
- Designed to allow plugins to modify the Rollup configuration before build
- Safely integrated with the existing plugin system
The hook system is initialized during Apibara instance creation and supports custom hooks through options.hooks, making it extensible for users while maintaining type safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for hook registrations and potential consumers
# Expected: Find files registering handlers for "rollup:before" hook
# Search for hook registrations
echo "Searching for rollup:before hook registrations..."
rg -l "rollup:before"
# Search for potential hook setup patterns
echo "Searching for hook setup patterns..."
ast-grep --pattern 'hooks.addHook($HOOK_NAME, $_)'
Length of output: 335
Script:
#!/bin/bash
# Check the hook definition and any existing consumers
echo "Checking hook definition in types/hooks.ts..."
cat packages/cli/src/types/hooks.ts
echo -e "\nSearching for any hook consumers or setup..."
rg -A 5 -B 5 "rollup:before"
# Look for any plugin or middleware registration patterns
echo -e "\nSearching for plugin registrations that might use hooks..."
rg -l "plugin|middleware|hooks?.register|hooks?.add"
Length of output: 3004
Script:
#!/bin/bash
# Check the Apibara class implementation for hook system
echo "Checking Apibara class implementation..."
cat packages/cli/src/core/apibara.ts
# Look for any plugin implementations that might use the rollup:before hook
echo -e "\nChecking rollup plugin implementations..."
cat packages/cli/src/rollup/plugins/indexers.ts
cat packages/cli/src/rollup/plugins/config.ts
Length of output: 2036
packages/cli/src/cli/index.ts (1)
12-12: LGTM! The start command integration follows the established pattern.
The implementation correctly lazy-loads the start command using dynamic import, consistent with other commands.
Let's verify the start command implementation exists:
✅ Verification successful
Start command implementation exists and follows the expected pattern
The verification confirms that the start command implementation exists at the expected location: packages/cli/src/cli/commands/start.ts, matching the import path in the CLI entry point.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the start command implementation exists
# Expect: The start command implementation file should exist
# Check if the start command implementation exists
fd --type f "start.ts$" "packages/cli/src/cli/commands"
Length of output: 94
packages/cli/src/rollup/plugins/indexers.ts (2)
1-2: LGTM: Imports are correctly typed and aligned with PR objectives.
The imports provide the necessary dependencies for creating a virtual Rollup plugin and ensuring proper TypeScript type checking.
7-15:
Add validation for indexer names and paths.
The virtual module generation could be more robust:
- Indexer names should be validated as they're used as variable names
- Indexer paths should be validated to prevent injection
- Consider adding error handling for failed imports
Let's verify the current usage of indexer names and paths:
Suggested improvement:
export function indexers(apibara: Apibara) {
if (!Array.isArray(apibara.indexers)) {
throw new Error('apibara.indexers must be an array');
}
+ // Validate indexer names and paths
+ apibara.indexers.forEach(({ name, indexer }) => {
+ if (!/^[a-zA-Z_$][0-9a-zA-Z_$]*$/.test(name)) {
+ throw new Error(`Invalid indexer name: ${name}`);
+ }
+ if (typeof indexer !== 'string' || !indexer.trim()) {
+ throw new Error(`Invalid indexer path for ${name}`);
+ }
+ });
const indexers = [...new Set(apibara.indexers)];
return virtual({
"#apibara-internal-virtual/indexers": `
+ // Handle potential import failures
+ const loadedIndexers = [];
${indexers.map((i) => `import ${i.name} from '${i.indexer}';`).join("\n")}
export const indexers = [
${indexers.map((i) => `{ name: "${i.name}", indexer: ${i.name} }`).join(",\n")}
];
`,
});
}packages/cli/tsconfig.json (2)
Line range hint 1-24: Verify removal of old internal path mappings.
The removal of apibara/internal/consola and apibara/internal/citty path mappings looks good, but we should verify no references remain.
Let's check for any remaining references:
#!/bin/bash
# Description: Verify no references remain to removed internal modules
# Test: Check for any remaining references to old internal paths
echo "Checking for remaining references to removed internal modules..."
rg -l "apibara/internal/(consola|citty)"
# Test: Check if the old internal directories were properly removed
echo "Checking if old internal directories were removed..."
fd -t d "consola|citty" src/internal16-17: LGTM! Path mappings align with the runtime refactoring.
The new path mappings are well-structured:
apibara/runtime/metafor runtime metadata#apibara-internal-virtual/*for build-time generated indexer lists
Let's verify the usage of these new paths:
✅ Verification successful
Path mappings are correctly configured and actively used
The verification confirms:
apibara/runtime/metais imported in build, dev, and start commands#apibara-internal-virtual/*is used in runtime/internal/app.ts for config and indexers- No references to old internal modules (consola/citty) remain
- Path mappings in tsconfig.json are consistent with the codebase structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of new path mappings and ensure they're properly referenced
# Test: Check if runtime-meta directory exists and is referenced
echo "Checking runtime-meta references..."
rg -l "apibara/runtime/meta"
# Test: Check for virtual type imports
echo "Checking virtual type imports..."
rg -l "#apibara-internal-virtual"
Length of output: 435
Script:
#!/bin/bash
# Let's verify the path mappings more thoroughly
# Check if runtime-meta directory exists
echo "Checking runtime-meta directory..."
fd -t d "runtime-meta"
# Check for any TypeScript/JavaScript files importing from runtime-meta
echo "Checking runtime-meta imports..."
rg -t ts -t js "from ['\"]apibara/runtime/meta['\"]"
rg -t ts -t js "require\(['\"]apibara/runtime/meta['\"]"
# Check for virtual type imports in TypeScript files
echo "Checking virtual type imports..."
rg -t ts "from ['\"]#apibara-internal-virtual/"
rg -t ts "require\(['\"]#apibara-internal-virtual/"
# Check for any remaining references to old internal modules
echo "Checking old internal module references..."
rg -t ts -t js "from ['\"]apibara/internal/(consola|citty)['\"]"
rg -t ts -t js "require\(['\"]apibara/internal/(consola|citty)['\"]"
# List all path mappings in tsconfig files to see the complete picture
echo "Checking all tsconfig path mappings..."
fd -t f "tsconfig.*json" --exec cat {} | jq -r '.compilerOptions.paths // empty'
Length of output: 1753
packages/cli/src/types/apibara.ts (1)
16-16: Verify indexers array initialization.
The addition of the indexers property aligns well with the PR objectives. However, we should verify how this array is initialized and handled in the implementation.
Let's check the initialization handling:
✅ Verification successful
Indexers array initialization is properly handled
The array is correctly initialized and managed throughout the codebase:
- Default initialization with empty array in
core/apibara.ts - Reset functionality in
core/scan.ts - Proper type definitions and usage in rollup plugins and CLI commands
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the indexers array is initialized and used
# Look for Apibara class/object initialization
ast-grep --pattern 'new Apibara({ $$ })' || ast-grep --pattern 'createApibara({ $$ })'
# Look for indexers array usage
rg -l "indexers\:" --type ts
Length of output: 158
Script:
#!/bin/bash
# Search for indexers property usage with correct regex
rg "indexers:" --type ts
# Look for Apibara interface/type usage
ast-grep --pattern 'interface Apibara {
$$$
}'
# Look for object creation or initialization with indexers
rg "indexers.*\[" --type ts -A 2
Length of output: 2525
packages/cli/src/cli/commands/build.ts (3)
2-2: LGTM: Clean import of runtime metadata.
The addition of runtimeDir import aligns with the PR's objective of managing indexers at build time.
20-20: LGTM: Standardized logging implementation.
The switch to apibara.logger aligns with the centralized logging approach being implemented across the codebase.
22-22: Verify the existence of start.mjs in runtime directory.
The entry point configuration looks correct, but let's verify the existence of the start.mjs file.
packages/cli/src/core/build/prod.ts (1)
8-8: LGTM! Verify logger initialization.
The logging message is clear and appropriate. The change aligns well with the centralized logging approach.
Let's verify the logger initialization:
✅ Verification successful
Logger is properly initialized and used consistently
The logger is correctly initialized in the createApibara function using consola.withTag("apibara") and is consistently used throughout the codebase. The usage in packages/cli/src/core/build/prod.ts follows the established pattern and is safe to use since the logger is guaranteed to be available as part of the Apibara instance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the logger is properly initialized before use
# Expect: Logger initialization in main CLI setup
# Search for logger initialization
rg -l "logger.*=.*createLogger|new.*Logger"
# Search for logger configuration
rg -l "logger.*config|configureLogger"
Length of output: 125
Script:
#!/bin/bash
# Let's check the actual logger initialization and usage patterns
# Check the content of the config update file first
cat packages/cli/src/core/config/update.ts
# Search for logger-related imports and usage
rg -A 3 "import.*logger|logger\."
# Search for apibara logger specifically
rg -A 3 "apibara.*logger"
Length of output: 9248
Script:
#!/bin/bash
# Let's check where the Apibara type is defined and how the logger is initialized
# Check the Apibara type definition
cat packages/cli/src/types.ts
# Also check main CLI entry points for initialization
rg -l "new Apibara|createApibara"
Length of output: 345
Script:
#!/bin/bash
# Let's examine the Apibara class and its initialization
# Check the core Apibara implementation
cat packages/cli/src/core/apibara.ts
# Check how it's used in the core index
cat packages/cli/src/core/index.ts
Length of output: 1197
packages/cli/src/core/apibara.ts (2)
11-11: LGTM: Clean import statement
The import statement for scanIndexers is properly placed with other imports and follows the project's import style.
Line range hint 13-36: Verify initialization order
The current initialization sequence is:
- Load options
- Create context with hooks
- Add hooks
- Scan indexers
This seems logical, but we should verify that scanIndexers doesn't depend on any hooks that might be added after its execution.
#!/bin/bash
# Check if scanIndexers uses any hooks
rg -A 5 "hooks\." src/core/scan.ts
# Check for any hook registrations related to indexers
rg -A 5 "addHook.*index" packages/cli/playground/indexers/starknet.indexer.ts (1)
13-13: Document the rationale for the new starting block.
The orderKey has been increased by 10x from 80,000 to 800,000. This significant change in the starting point needs documentation to explain:
- Why this specific block number was chosen
- Whether any important historical data between these blocks can be skipped
- Impact on existing data processing pipelines
packages/cli/build.config.ts (3)
5-13: LGTM: Runtime module addition
The addition of the "runtime" module to the modules array is clean and maintains alphabetical order.
40-43: LGTM: Externals configuration
The externals configuration correctly includes the new runtime meta module and maintains consistency with the alias configuration.
26-26: Verify the runtime module structure
The runtime module uses a directory-based input which suggests it may contain multiple files. Let's verify the structure and ensure all necessary files are included in the build.
✅ Verification successful
Runtime module structure is well-organized and complete
The runtime module structure is properly organized with:
- Main entry point:
src/runtime/index.ts - Core functionality files:
dev.ts,start.ts - Internal components:
internal/app.ts
The build configuration correctly includes the entire runtime directory for ESM format bundling, which will package all these files appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the runtime module structure and contents
# Check the runtime source directory structure
echo "Runtime source directory structure:"
fd . "src/runtime" -t f
# Check for any potential entry points or index files
echo -e "\nPotential entry points:"
rg -l "export" "src/runtime"
Length of output: 345
Script:
#!/bin/bash
# Let's first check if the runtime directory exists in the repository
echo "Searching for runtime directory:"
fd runtime
# Check for any files with 'runtime' in their path
echo -e "\nFiles with 'runtime' in path:"
fd . -t f | rg -i runtime
# Let's also check the build config file to understand the context better
echo -e "\nBuild config content:"
cat packages/cli/build.config.ts
Length of output: 1921
packages/cli/src/core/scan.ts (1)
1-40: Verify indexer file naming consistency across the codebase.
Let's ensure all indexer files follow the expected naming pattern.
✅ Verification successful
Let me try a different approach to verify the indexer files in the codebase.
Based on the results, I can now generate a final response.
The .indexer.ts naming convention is correctly enforced
Looking at the codebase, all actual indexer files follow the .indexer.ts naming convention as seen in the examples:
examples/cli/indexers/starknet.indexer.tspackages/cli/playground/indexers/starknet.indexer.ts
Other files that import or use indexer-related code are utility files, tests, or implementation files that support the indexer functionality but are not indexers themselves. The scanIndexers function's naming convention check is appropriate and working as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential indexer files that don't follow the naming convention
# Find all .ts and .js files that might be indexers but don't follow the convention
echo "Files that might be indexers but don't follow the naming convention:"
rg -g "*.{ts,js}" -l "export\s+class.*extends\s+Indexer" | grep -v "\.indexer\.(ts|js)$" || true
# List all properly named indexer files for reference
echo -e "\nProperly named indexer files:"
fd -e ts -e js . | grep "\.indexer\.(ts|js)$" || true
Length of output: 377
Script:
#!/bin/bash
# Let's check for any class that extends Indexer to find all indexer files
echo "All files containing Indexer classes:"
ast-grep --pattern 'class $_ extends Indexer { $$$ }'
# Also check for imports of Indexer to ensure we don't miss any
echo -e "\nFiles importing Indexer:"
rg "import.*Indexer" -A 2
Length of output: 6978
packages/cli/src/runtime/start.ts (2)
1-4: LGTM: Clean and well-organized imports.
The imports are appropriately scoped, with a clear separation between external dependencies and internal modules.
4-4: Verify integration with Rollup's virtual plugin.
The PR objectives mention using Rollup's virtual plugin to generate indexer lists at build time. Let's verify how createIndexer integrates with this system.
Also applies to: 25-25
✅ Verification successful
Let me gather more information about the virtual plugin implementation and its connection to the indexer.
Integration with Rollup's virtual plugin is properly implemented
The virtual plugin integration is correctly implemented:
- The
indexers.tsplugin generates a virtual module that imports and exports all registered indexers - The
createIndexerfunction inapp.tsworks with the configuration provided by the virtual config plugin - Both plugins use
@rollup/plugin-virtualto inject the required code at build time
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how createIndexer is implemented and where the indexer list is generated
echo "Checking createIndexer implementation:"
ast-grep --pattern 'function createIndexer' -A 10
echo "\nChecking Rollup configuration for virtual plugin:"
rg -l "rollup.*virtual" --type ts
Length of output: 373
Script:
#!/bin/bash
# Check the virtual plugin implementation in indexers.ts
echo "Checking virtual plugin implementation:"
cat packages/cli/src/rollup/plugins/indexers.ts
echo "\nChecking createIndexer implementation in app.ts:"
rg -A 10 "export function createIndexer" packages/cli/src/runtime/internal/app.ts
echo "\nChecking config plugin usage:"
cat packages/cli/src/rollup/plugins/config.ts
Length of output: 1569
packages/cli/src/runtime/internal/app.ts (2)
1-6: LGTM! Clean imports and useful constant export.
The imports are well-organized, and the availableIndexers constant provides a convenient way to access the list of available indexers.
3-4: Verify virtual module setup.
The code relies on virtual modules generated at build time. Let's verify their setup.
✅ Verification successful
Virtual module setup is correctly implemented
The virtual modules are properly configured:
- Virtual module plugins are correctly implemented in
packages/cli/src/rollup/plugins/for bothconfigandindexers - Type definitions are present in
packages/cli/src/types/virtual/ - The virtual imports are properly aliased in the TypeScript configuration
- Both virtual modules export the expected constants with correct types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the virtual modules are properly configured in the build setup
# Check for virtual module configuration in rollup config
rg -l "apibara-internal-virtual" -g "rollup.config.*"
# Check for the indexers virtual module implementation
rg "export const indexers" -A 5
Length of output: 763
Script:
#!/bin/bash
# Let's check for the virtual module configuration in the build setup more thoroughly
# Check for virtual module aliases or configuration in package.json
rg "apibara-internal-virtual" -g "package.json"
# Check for virtual module setup in build configuration files
fd -e js -e ts -e json "rollup|vite|webpack|tsconfig" -x rg "apibara-internal-virtual" {}
# Check the config virtual module implementation
rg "export const config" -A 5
# Look for virtual module plugin setup
ast-grep --pattern 'virtual($$$)'
Length of output: 1673
packages/cli/src/types/config.ts (4)
18-22: LGTM! Improved type safety with Record<string, never>
The change from empty object type to Record<string, never> is a good improvement. This makes the type more explicit and prevents accidental property access, which was possible with the empty object type.
41-45: LGTM! Consistent type parameter changes
The type parameter changes mirror those in ApibaraConfig, maintaining consistency throughout the type system.
Line range hint 18-74: Document breaking changes in CHANGELOG
This update introduces breaking changes:
- Type parameter defaults changed from empty object type to
Record<string, never> - New required
indexersDirproperty inApibaraOptions
Please ensure these changes are documented in the CHANGELOG.
#!/bin/bash
# Description: Check if CHANGELOG exists and contains relevant entries
# Test: Look for CHANGELOG files
fd -g 'CHANGELOG*' -t f74-74: Document sourceMap property and verify build configuration
Please add documentation for the sourceMap property. Also, ensure that the Rollup configuration correctly handles this option.
packages/cli/src/core/build/dev.ts (1)
56-56: LGTM! Verify hook implementation.
The addition of the dev:restart hook is well-placed, being called at the start of the Rollup watcher lifecycle. This aligns with the PR's objective of refactoring the build commands.
Let's verify the hook implementation and its consumers:
✅ Verification successful
✓ Hook implementation verified and properly used
The dev:restart hook is correctly implemented:
- Defined in types:
packages/cli/src/types/hooks.ts - Called in build process:
packages/cli/src/core/build/dev.ts - Consumed in dev command:
packages/cli/src/cli/commands/dev.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the dev:restart hook implementation and usage
# Expected: Find the hook definition and any consumers
# Test 1: Find the hook definition in types
echo "Searching for hook definition..."
rg -t typescript "dev:restart" --glob "**/types/**"
# Test 2: Find hook consumers
echo "Searching for hook consumers..."
rg -t typescript "dev:restart" --glob "!**/build/dev.ts"
Length of output: 326
Script:
#!/bin/bash
# Let's try with ripgrep without type filtering since typescript type is not recognized
echo "Searching for hook definition..."
rg "dev:restart" --glob "**/*.{ts,tsx,d.ts}"
# Let's also check for any hook registration patterns
echo "Searching for hook registration patterns..."
ast-grep --pattern 'hooks.addHook("dev:restart"'
Length of output: 495
packages/cli/package.json (4)
66-66: LGTM: Enhanced debugging capability.
The addition of CONSOLA_LEVEL=debug improves the development experience by providing more detailed logging information.
Line range hint 2-2: Verify breaking changes documentation for v2 release.
The version bump to 2.0.0-beta.11 indicates breaking changes. Please ensure these changes are properly documented.
#!/bin/bash
# Description: Check for breaking changes documentation
# Look for changelog or migration guide
echo "Checking for changelog or migration guide..."
fd -t f "CHANGELOG|MIGRATION|BREAKING" --max-depth 2
# Check for breaking change documentation in README
echo "Checking README for breaking changes..."
rg -i "breaking.?changes?|migration|upgrade" "README.md"35-49: Verify runtime exports structure and patterns.
The new runtime exports follow good practices for dual package hazard prevention by explicitly declaring types. However, let's verify that all runtime modules are properly structured.
✅ Verification successful
Runtime exports are properly structured and used consistently
The runtime exports in package.json are correctly configured and align with the actual codebase structure:
- Runtime meta files exist at the package root (
runtime-meta.d.ts,runtime-meta.mjs) - Runtime-related source files are properly organized under
src/ - Runtime imports are consistently used across build, dev, and start commands
- The exports follow the dual package hazard prevention pattern with proper type declarations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify runtime directory structure and exports
# Check if runtime directory exists and contains expected files
echo "Checking runtime directory structure..."
fd -t f -e mjs -e d.ts . "packages/cli/dist/runtime"
# Verify no unexpected runtime exports
echo "Checking for unexpected runtime exports..."
rg -g "!package.json" "from ['\"]\\.\\./runtime" "packages/cli"
Length of output: 413
Script:
#!/bin/bash
# Let's verify the runtime-related files and their structure differently
# Check for runtime-related files in the CLI package
echo "Checking runtime-related files in CLI package..."
fd -t f "runtime" "packages/cli"
# Look for runtime imports/exports patterns
echo "Checking runtime import/export patterns..."
rg "runtime" "packages/cli/package.json" -A 2 -B 2
# Check source files that might use runtime
echo "Checking runtime usage in source files..."
rg "from ['\"].*runtime" "packages/cli/src"
Length of output: 1563
73-73: Verify dependency changes and usage.
The dependency changes align with the PR objectives, particularly the addition of Rollup plugins. However, let's verify their usage:
Also applies to: 89-89, 103-103
✅ Verification successful
Dependencies are correctly placed and actively used
The verification shows that:
@rollup/plugin-virtualis actively used inplugins/indexers.tsandplugins/config.tsrollup-plugin-esbuildis used inrollup/config.ts@apibara/starknetis used in playground examples and makes sense as a dev dependency since it's used for testing/development purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify new dependencies are being used
echo "Checking @rollup/plugin-virtual usage..."
rg "from ['\"]\@rollup/plugin-virtual"
echo "Checking rollup-plugin-esbuild usage..."
rg "from ['\"](rollup-plugin-esbuild|esbuild)"
echo "Checking @apibara/starknet usage in tests..."
rg "from ['\"]\@apibara/starknet"
Length of output: 299
Script:
#!/bin/bash
# Let's try a more thorough search for these dependencies
echo "Checking @rollup/plugin-virtual imports..."
rg -l "[@]rollup/plugin-virtual"
echo "Checking rollup-plugin-esbuild imports..."
rg -l "rollup-plugin-esbuild"
echo "Checking direct esbuild imports..."
rg -l "esbuild['\"]"
echo "Checking @apibara/starknet imports..."
rg -l "[@]apibara/starknet"
# Let's also check package.json to see where these are listed
echo "Checking package.json entries..."
fd "package.json" --exec grep -l "rollup\|esbuild\|apibara"
Length of output: 1810
Script:
#!/bin/bash
# Let's check the actual usage context of these dependencies
echo "Checking @rollup/plugin-virtual usage context..."
rg -A 5 "[@]rollup/plugin-virtual" packages/cli/src/rollup/plugins/
echo "Checking rollup-plugin-esbuild usage context..."
rg -A 5 "rollup-plugin-esbuild" packages/cli/src/rollup/config.ts
echo "Checking @apibara/starknet usage in CLI package..."
rg -A 5 "[@]apibara/starknet" packages/cli/
Length of output: 2523
packages/cli/src/cli/commands/dev.ts (4)
3-3: LGTM! Import changes enhance cross-platform compatibility.
The addition of runtimeDir from runtime metadata and using pathe for path operations improves the code's portability.
Also applies to: 6-6
35-38: LGTM! Improved variable scoping.
Moving apibara and childProcess declarations into the function scope enhances encapsulation and prevents potential issues with shared state.
41-44: LGTM! Enhanced logging and proper resource cleanup.
The implementation properly handles:
- Centralized logging through
apibara.logger - Cleanup of watchers before reloading
- Configuration of the entry point to use build-time generated files
Also applies to: 79-79
85-91: LGTM! Well-structured hook for indexer lifecycle management.
The new "dev:restart" hook provides clean shutdown of indexers before restart, preventing orphaned processes.
packages/cli/src/core/build/types.ts (1)
9-9: LGTM: Logging changes align with centralization effort.
The replacement of consola with apibara.logger and the addition of a start message improve the logging consistency and provide better visibility into the type generation process.
Also applies to: 134-134
packages/cli/src/rollup/config.ts (3)
75-76: LGTM on adding indexers and appConfig plugins
The addition of the indexers and appConfig plugins to the Rollup configuration enhances the build process by incorporating dynamic indexer imports and application configuration. The plugins are correctly implemented and integrated.
44-46:
Reconsider ignoring the THIS_IS_UNDEFINED warning
The onwarn function now ignores the THIS_IS_UNDEFINED warning. Suppressing this warning might hide potential issues where this is undefined in ES module contexts. Please ensure that ignoring this warning does not mask unintended behavior, especially in strict mode or when targeting newer JavaScript versions.
Run the following script to identify occurrences where this might be used in a way that could lead to undefined values:
#!/bin/bash
# Description: Find top-level usages of `this` which may cause `THIS_IS_UNDEFINED` warnings.
# Test: Search for top-level `this` usage. Expectation: No top-level `this` references.
rg --type js --type ts --glob '!node_modules/*' -U -w '\bthis\b' .13-13: Update function signature and ensure dependent code is updated
The getRollupConfig function signature has changed with the removal of the dev parameter. Please ensure that all calls to this function are updated to match the new signature to prevent any runtime errors.
Run the following script to identify any function calls that still use the old signature:
✅ Verification successful
Let me try a different search pattern to find calls to getRollupConfig:
No updates needed for function calls
The search results show that getRollupConfig is only called once in packages/cli/src/core/build/build.ts and it's already using the new signature with a single apibara parameter. No other calls to this function exist in the codebase, so no updates are required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to getRollupConfig with more than one argument.
# Test: Search for function calls to getRollupConfig with at least two arguments.
rg 'getRollupConfig\(\s*[^,\s]+,\s*[^,\s]+\s*\)'
Length of output: 12495
Script:
#!/bin/bash
# Search for any calls to getRollupConfig function
rg "getRollupConfig\(" --type ts --type js -A 2
Length of output: 497
| export async function resolveRuntimeConfigOptions(options: ApibaraOptions) { | ||
| options.runtimeConfig = { ...options.runtimeConfig, default: "value" }; | ||
| options.runtimeConfig = { ...options.runtimeConfig }; | ||
| process.env.APIBARA_RUNTIME_CONFIG = JSON.stringify(options.runtimeConfig); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding runtime type validation.
Since this function handles configuration that affects runtime behavior, it would be beneficial to add runtime type validation to ensure the configuration object meets expected requirements.
Consider using a validation library like zod or implementing a type guard:
interface RuntimeConfig {
// define expected runtime config properties
}
function isValidRuntimeConfig(config: unknown): config is RuntimeConfig {
// implement validation logic
return typeof config === 'object' && config !== null;
}
export async function resolveRuntimeConfigOptions(options: ApibaraOptions) {
const runtimeConfig = options.runtimeConfig ?? {};
if (!isValidRuntimeConfig(runtimeConfig)) {
throw new Error('Invalid runtime configuration provided');
}
options.runtimeConfig = { ...runtimeConfig };
process.env.APIBARA_RUNTIME_CONFIG = JSON.stringify(options.runtimeConfig);
}| options.runtimeConfig = { ...options.runtimeConfig }; | ||
| process.env.APIBARA_RUNTIME_CONFIG = JSON.stringify(options.runtimeConfig); |
There was a problem hiding this comment.
Add null check for runtimeConfig property.
The current implementation might throw an error if options.runtimeConfig is undefined. Consider adding a default empty object to ensure safe property spreading.
Apply this diff to add null safety:
- options.runtimeConfig = { ...options.runtimeConfig };
+ options.runtimeConfig = { ...(options.runtimeConfig ?? {}) };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| options.runtimeConfig = { ...options.runtimeConfig }; | |
| process.env.APIBARA_RUNTIME_CONFIG = JSON.stringify(options.runtimeConfig); | |
| options.runtimeConfig = { ...(options.runtimeConfig ?? {}) }; | |
| process.env.APIBARA_RUNTIME_CONFIG = JSON.stringify(options.runtimeConfig); |
| export async function updateApibaraConfig( | ||
| apibara: Apibara, | ||
| config: ApibaraDynamicConfig, | ||
| _config: ApibaraDynamicConfig, |
There was a problem hiding this comment.
Consider removing the unused parameter.
The _config parameter is marked as unused with an underscore prefix. If this parameter isn't needed for the function's operation, consider removing it to improve code clarity.
export async function updateApibaraConfig(
- apibara: Apibara,
- _config: ApibaraDynamicConfig,
+ apibara: Apibara,
) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _config: ApibaraDynamicConfig, | |
| apibara: Apibara, |
| export async function updateApibaraConfig( | ||
| apibara: Apibara, | ||
| config: ApibaraDynamicConfig, | ||
| _config: ApibaraDynamicConfig, | ||
| ) { | ||
| await apibara.hooks.callHook("rollup:reload"); | ||
| consola.success("Apibara config hot reloaded!"); | ||
| apibara.logger.success("Apibara config hot reloaded!"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function name could be more descriptive.
The function name updateApibaraConfig suggests it updates configuration, but it actually triggers a Rollup reload and doesn't use the config parameter. Consider renaming to better reflect its purpose, such as reloadRollupConfig or triggerConfigReload.
-export async function updateApibaraConfig(
+export async function reloadRollupConfig(
apibara: Apibara,
- _config: ApibaraDynamicConfig,
) {
await apibara.hooks.callHook("rollup:reload");
apibara.logger.success("Apibara config hot reloaded!");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function updateApibaraConfig( | |
| apibara: Apibara, | |
| config: ApibaraDynamicConfig, | |
| _config: ApibaraDynamicConfig, | |
| ) { | |
| await apibara.hooks.callHook("rollup:reload"); | |
| consola.success("Apibara config hot reloaded!"); | |
| apibara.logger.success("Apibara config hot reloaded!"); | |
| } | |
| export async function reloadRollupConfig( | |
| apibara: Apibara, | |
| ) { | |
| await apibara.hooks.callHook("rollup:reload"); | |
| apibara.logger.success("Apibara config hot reloaded!"); | |
| } |
| fromEnv: { | ||
| nodeEnv: process.env.NODE_ENV, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding type safety and validation for environment variables.
The direct use of process.env.NODE_ENV could lead to runtime issues if the environment variable is undefined. Consider:
- Adding type safety
- Providing a default value
- Validating allowed values
fromEnv: {
- nodeEnv: process.env.NODE_ENV,
+ nodeEnv: process.env.NODE_ENV ?? 'development',
},You might also want to add a type guard:
const validateNodeEnv = (env: string | undefined): env is 'development' | 'production' | 'test' => {
return env === 'development' || env === 'production' || env === 'test';
};
// Usage
nodeEnv: validateNodeEnv(process.env.NODE_ENV)
? process.env.NODE_ENV
: 'development',| apibara.logger.start("Building"); | ||
|
|
||
| apibara.options.entry = join(runtimeDir, "start.mjs"); | ||
|
|
||
| await prepare(apibara); | ||
| await writeTypes(apibara); | ||
| await build(apibara); | ||
| await apibara.close(); |
There was a problem hiding this comment.
Add error handling and improve logging.
The build process lacks error handling and could benefit from more detailed logging to help diagnose build failures.
-apibara.logger.start("Building");
+apibara.logger.start("Starting build process");
apibara.options.entry = join(runtimeDir, "start.mjs");
+try {
+ apibara.logger.info("Preparing build environment");
await prepare(apibara);
+ apibara.logger.info("Generating TypeScript types");
await writeTypes(apibara);
+ apibara.logger.info("Building indexer");
await build(apibara);
+ apibara.logger.success("Build completed successfully");
+} catch (error) {
+ apibara.logger.error("Build failed", error);
+ process.exit(1);
+}
await apibara.close();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| apibara.logger.start("Building"); | |
| apibara.options.entry = join(runtimeDir, "start.mjs"); | |
| await prepare(apibara); | |
| await writeTypes(apibara); | |
| await build(apibara); | |
| await apibara.close(); | |
| apibara.logger.start("Starting build process"); | |
| apibara.options.entry = join(runtimeDir, "start.mjs"); | |
| try { | |
| apibara.logger.info("Preparing build environment"); | |
| await prepare(apibara); | |
| apibara.logger.info("Generating TypeScript types"); | |
| await writeTypes(apibara); | |
| apibara.logger.info("Building indexer"); | |
| await build(apibara); | |
| apibara.logger.success("Build completed successfully"); | |
| } catch (error) { | |
| apibara.logger.error("Build failed", error); | |
| process.exit(1); | |
| } | |
| await apibara.close(); |
| spawn("node", childArgs, { | ||
| stdio: "inherit", | ||
| }); |
There was a problem hiding this comment.
Implement proper process management.
The child process spawning lacks error handling and proper cleanup mechanisms. This could lead to orphaned processes or unhandled errors.
-spawn("node", childArgs, {
- stdio: "inherit",
-});
+const child = spawn("node", childArgs, {
+ stdio: "inherit",
+});
+
+child.on("error", (error) => {
+ apibara.logger.error("Failed to start indexer", error);
+ process.exit(1);
+});
+
+// Handle parent process exit
+process.on("SIGTERM", () => {
+ child.kill("SIGTERM");
+});
+
+process.on("SIGINT", () => {
+ child.kill("SIGINT");
+});
+
+child.on("exit", (code) => {
+ process.exit(code ?? 0);
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| spawn("node", childArgs, { | |
| stdio: "inherit", | |
| }); | |
| const child = spawn("node", childArgs, { | |
| stdio: "inherit", | |
| }); | |
| child.on("error", (error) => { | |
| apibara.logger.error("Failed to start indexer", error); | |
| process.exit(1); | |
| }); | |
| // Handle parent process exit | |
| process.on("SIGTERM", () => { | |
| child.kill("SIGTERM"); | |
| }); | |
| process.on("SIGINT", () => { | |
| child.kill("SIGINT"); | |
| }); | |
| child.on("exit", (code) => { | |
| process.exit(code ?? 0); | |
| }); |
|
|
||
| if (dev) { | ||
| options.dev = dev; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent object shape by always setting the dev flag.
Currently, options.dev is only set when dev is true. This can lead to inconsistent object shapes which might cause issues with type checking or downstream code that expects the property to exist.
Consider always setting the flag:
- if (dev) {
- options.dev = dev;
- }
+ options.dev = dev;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (dev) { | |
| options.dev = dev; | |
| } | |
| options.dev = dev; | |
| selectedIndexers.map(async (indexer) => { | ||
| const indexerInstance = createIndexer(indexer, preset); | ||
|
|
||
| const client = createClient( | ||
| indexerInstance.streamConfig, | ||
| indexerInstance.options.streamUrl, | ||
| ); | ||
|
|
||
| await run(client, indexerInstance); | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance user feedback by adding logging when indexers start and finish
Adding logging statements will help users track which indexers are running and when they complete.
Apply this diff to add logging:
selectedIndexers.map(async (indexer) => {
+ console.log(`Starting indexer: ${indexer}`);
const indexerInstance = createIndexer(indexer, preset);
const client = createClient(
indexerInstance.streamConfig,
indexerInstance.options.streamUrl,
);
await run(client, indexerInstance);
+ console.log(`Indexer ${indexer} has completed.`);
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| selectedIndexers.map(async (indexer) => { | |
| const indexerInstance = createIndexer(indexer, preset); | |
| const client = createClient( | |
| indexerInstance.streamConfig, | |
| indexerInstance.options.streamUrl, | |
| ); | |
| await run(client, indexerInstance); | |
| }), | |
| selectedIndexers.map(async (indexer) => { | |
| console.log(`Starting indexer: ${indexer}`); | |
| const indexerInstance = createIndexer(indexer, preset); | |
| const client = createClient( | |
| indexerInstance.streamConfig, | |
| indexerInstance.options.streamUrl, | |
| ); | |
| await run(client, indexerInstance); | |
| console.log(`Indexer ${indexer} has completed.`); | |
| }), |
| await Promise.all( | ||
| selectedIndexers.map(async (indexer) => { | ||
| const indexerInstance = createIndexer(indexer, preset); | ||
|
|
||
| const client = createClient( | ||
| indexerInstance.streamConfig, | ||
| indexerInstance.options.streamUrl, | ||
| ); | ||
|
|
||
| await run(client, indexerInstance); | ||
| }), | ||
| ); | ||
| }, |
There was a problem hiding this comment.
Handle errors when running indexers in parallel
Currently, if one indexer fails, the Promise.all will reject, causing all indexers to stop. Consider handling errors for individual indexers to ensure that one failure doesn't interrupt the execution of other indexers.
Apply this diff to handle errors gracefully:
- await Promise.all(
+ await Promise.allSettled(
selectedIndexers.map(async (indexer) => {
const indexerInstance = createIndexer(indexer, preset);
const client = createClient(
indexerInstance.streamConfig,
indexerInstance.options.streamUrl,
);
- await run(client, indexerInstance);
+ try {
+ await run(client, indexerInstance);
+ } catch (error) {
+ console.error(`Indexer ${indexer} failed with error:`, error);
+ }
}),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await Promise.all( | |
| selectedIndexers.map(async (indexer) => { | |
| const indexerInstance = createIndexer(indexer, preset); | |
| const client = createClient( | |
| indexerInstance.streamConfig, | |
| indexerInstance.options.streamUrl, | |
| ); | |
| await run(client, indexerInstance); | |
| }), | |
| ); | |
| }, | |
| await Promise.allSettled( | |
| selectedIndexers.map(async (indexer) => { | |
| const indexerInstance = createIndexer(indexer, preset); | |
| const client = createClient( | |
| indexerInstance.streamConfig, | |
| indexerInstance.options.streamUrl, | |
| ); | |
| try { | |
| await run(client, indexerInstance); | |
| } catch (error) { | |
| console.error(`Indexer ${indexer} failed with error:`, error); | |
| } | |
| }), | |
| ); | |
| }, |
| await prepare(apibara); | ||
| await writeTypes(apibara); | ||
| await build(apibara); |
There was a problem hiding this comment.
IMHO when user runs apibara start command, it should just run the build files generated from apibara build and not build it again and run it. What you think?
There was a problem hiding this comment.
Yeah I was unsure about this too. I think you're right and we should print an error message if the user hasn't built the app first (like Next.js does, message included for reference).
▲ Next.js 15.0.2
- Local: http://localhost:3000
✓ Starting...
Error: Could not find a production build in the '.next' directory. Try building your app with 'next build' before starting the production server. https://nextjs.org/docs/messages/production-start-no-build-id
at setupFsCheck (/home/fra/.local/share/pnpm/store/v3/tmp/dlx-122690/node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/next/dist/server/lib/router-utils/filesystem.js:149:19)
at async initialize (/home/fra/.local/share/pnpm/store/v3/tmp/dlx-122690/node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/next/dist/server/lib/router-server.js:65:23)
at async Server.<anonymous> (/home/fra/.local/share/pnpm/store/v3/tmp/dlx-122690/node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/next/dist/server/lib/start-server.js:266:36)
There was a problem hiding this comment.
BTW love that Next.js has an "errors" section in their docs. We should do that too.
There was a problem hiding this comment.
We should improve the output of the build command to match nitro.
ℹ Using 2024-04-03 as fallback. nitro 19:39:25
Please specify compatibility date to avoid unwanted behavior changes:
- Add compatibilityDate: '2024-11-02' to the config file.
- Or set COMPATIBILITY_DATE=2024-11-02 environment variable.
✔ Generated public .output/public nitro 19:39:25
ℹ Building Nitro Server (preset: node-server, compatibility date: 2024-04-03) nitro 19:39:25
✔ Nitro Server built nitro 19:39:26
├─ .output/server/chunks/nitro/nitro.mjs (163 kB) (39 kB gzip)
├─ .output/server/chunks/nitro/nitro.mjs.map (4.61 kB) (875 B gzip)
├─ .output/server/chunks/routes/index.mjs (310 B) (212 B gzip)
├─ .output/server/chunks/routes/index.mjs.map (208 B) (150 B gzip)
├─ .output/server/index.mjs (267 B) (183 B gzip)
└─ .output/server/package.json (89 B) (95 B gzip)
Σ Total size: 168 kB (40.5 kB gzip)
✔ You can preview this build using node .output/server/index.mjs
|
I updated the PR to fix that issue and improve the output. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
packages/cli/src/core/path.ts (2)
4-11: LGTM! Consider enhancing the documentation.The implementation is clean and focused. However, the documentation could be more comprehensive.
Consider expanding the JSDoc comment like this:
/** Return a (possibly highlighted) path relative to the current working directory. * + * @param path - The absolute or relative path to process + * @param highlight - Whether to apply cyan color highlighting to the path + * @returns The path relative to CWD, optionally highlighted in cyan * * From nitrojs/nitro. */
8-11: Consider adding input validation.The function could benefit from basic input validation to handle edge cases gracefully.
Consider adding validation:
export function prettyPath(path: string, highlight = true) { + if (typeof path !== 'string' || path.trim().length === 0) { + throw new Error('Path must be a non-empty string'); + } const rel = relative(process.cwd(), path); return highlight ? colors.cyan(rel) : rel; }examples/cli/package.json (1)
10-11: Consider adding format command for consistency.While the lint and lint:fix commands are correctly configured, consider adding a separate format command for better separation of concerns. Biome supports both linting and formatting, and it's a common practice to separate these operations.
Add these commands:
"scripts": { "build": "apibara build", "dev": "apibara dev", "start": "apibara start", "lint": "biome check .", - "lint:fix": "pnpm lint --write" + "lint:fix": "pnpm lint --apply", + "format": "biome format .", + "format:fix": "pnpm format --write" },examples/cli/indexers/starknet.indexer.ts (2)
Line range hint
27-29: Align logging with new apibara logger frameworkThe transform function is using
console.logwhile the codebase is moving towards using theapibaralogging framework. Consider updating this to maintain consistency with the new logging standards.- async transform({ block: { header } }) { - console.log("Transforming block ", header?.blockNumber); + async transform({ block: { header } }, { logger }) { + logger.info("transforming block", { blockNumber: header?.blockNumber }); },
Line range hint
6-6: Update runtime config logging to use apibara loggerThe runtime config logging should also use the new
apibaralogging framework instead ofconsole.log.- console.log("--> Starknet Indexer Runtime Config: ", runtimeConfig); + runtimeConfig.logger.debug("starknet indexer runtime config", { config: runtimeConfig });package.json (1)
Line range hint
1-47: Consider addressing the removed format script.The format script has been removed, but code formatting is still important. Consider either:
- Adding format functionality to the lint:fix script
- Documenting how formatting should be handled
- Re-implementing the format script using pnpm
Would you like me to propose an implementation for a new format script using pnpm?
packages/cli/src/cli/commands/start.ts (1)
25-38: Consider adding root directory validation and centralizing path configuration.While the setup works, consider these improvements:
- Validate that the root directory exists and is accessible
- Move the
.apibara/buildpath to a centralized configurationconst rootDir = resolve((args.dir || args._dir || ".") as string); +if (!fse.existsSync(rootDir)) { + apibara.logger.error(`Directory ${rootDir} does not exist`); + process.exit(1); +} const apibara = await createApibara({ rootDir, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
examples/cli/indexers/starknet.indexer.ts(1 hunks)examples/cli/package.json(1 hunks)flake.nix(1 hunks)package.json(1 hunks)packages/cli/package.json(4 hunks)packages/cli/src/cli/commands/build.ts(2 hunks)packages/cli/src/cli/commands/dev.ts(5 hunks)packages/cli/src/cli/commands/start.ts(1 hunks)packages/cli/src/core/build/prepare.ts(1 hunks)packages/cli/src/core/build/prod.ts(2 hunks)packages/cli/src/core/build/types.ts(2 hunks)packages/cli/src/core/path.ts(1 hunks)packages/cli/src/core/scan.ts(1 hunks)packages/indexer/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/cli/src/cli/commands/build.ts
- packages/cli/src/cli/commands/dev.ts
- packages/cli/src/core/build/prepare.ts
- packages/cli/src/core/build/prod.ts
- packages/cli/src/core/build/types.ts
- packages/cli/src/core/scan.ts
🔇 Additional comments (12)
examples/cli/package.json (1)
7-9: Verify the apibara CLI commands exist.
The new build, dev, and start commands align with the PR objectives of refactoring these commands.
Let's verify these commands are properly exported:
✅ Verification successful
The apibara CLI commands are properly implemented
The verification confirms that the build, dev, and start commands are properly implemented in the CLI:
- Commands are defined in
packages/cli/src/cli/commands/:build.ts- implements the build commanddev.ts- implements the dev commandstart.ts- implements the start command
- All commands are properly registered in
packages/cli/src/cli/index.tsusing thecittyframework - The CLI is exposed through the
binentry inpackages/cli/package.jsonasapibara
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the apibara CLI commands exist in the codebase
# Test: Search for command definitions
rg -A 5 'new Command\(' 'packages/cli/src'
Length of output: 42
Script:
#!/bin/bash
# Let's try different approaches to find CLI command definitions
# Search for command-related files and patterns
echo "=== Searching for command files ==="
fd -t f . packages/cli/src
echo -e "\n=== Searching for command patterns ==="
rg -i "command|program" packages/cli/src
echo -e "\n=== Checking package.json for bin entries ==="
cat packages/cli/package.json
echo -e "\n=== Looking for CLI entry points ==="
rg -l "#!/usr/bin/env node" .
Length of output: 6924
flake.nix (1)
16-16: Please clarify the security implications of enabling MADNESS_ALLOW_LDD.
The addition of this environment variable raises several concerns:
- The variable name suggests it enables
lddfunctionality, which could have security implications if misused. - The "MADNESS" prefix suggests this might be a temporary workaround.
Could you please:
- Document the purpose of this variable
- Confirm if this is a temporary solution
- Explain any security considerations
Let's check if this variable is documented or used elsewhere:
examples/cli/indexers/starknet.indexer.ts (2)
Line range hint 7-26: Core indexer configuration looks good
The indexer configuration is well-structured with proper filter setup for Transfer events. The use of type-safe contract address and event selector calculation is a good practice.
12-12: Verify the significant increase in orderKey
The orderKey has been increased by 10x (from 80,000n to 800,000n). This significant jump in the starting point could potentially skip important historical data. Please confirm if this increase is intentional and won't impact data consistency or dependent systems.
✅ Verification successful
The orderKey change is part of a coordinated update
Based on the git history, the change from 80,000n to 800,000n is part of a coordinated update across multiple Starknet indexer configurations. The change appears in multiple files and was implemented consistently:
- examples/cli/indexers/starknet.indexer.ts
- examples/starknet-client/src/main.ts
- packages/cli/playground/indexers/starknet.indexer.ts
The change also coincides with updates to the streamUrl from a local endpoint to the official preview endpoint (https://starknet.preview.apibara.org), suggesting this was part of a planned infrastructure upgrade.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other orderKey configurations to ensure consistency
rg -g '*.ts' -g '*.js' --no-heading "orderKey.*800_000n"
Length of output: 270
Script:
#!/bin/bash
# Let's check the git history to understand if this change was coordinated across files
git log -p --all | rg -A 5 -B 5 "orderKey.*[0-9]+_000n"
Length of output: 15829
package.json (1)
12-13: Verify CI/CD pipeline compatibility with pnpm lint commands.
While switching from turbo to pnpm for lint commands is valid, ensure that CI/CD pipelines and documentation are updated accordingly.
✅ Verification successful
CI/CD pipeline is already compatible with pnpm lint commands
The CI/CD pipeline in .github/workflows/build.yml is already using pnpm lint command, and all other workflow files are consistently using pnpm. The change from turbo to pnpm for lint commands is properly aligned with the existing CI/CD configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any CI/CD configurations that might reference the old turbo lint commands
# Test: Search for turbo lint references in CI configs
rg -g '*.{yml,yaml}' 'turbo.*lint' .github/
Length of output: 45
Script:
#!/bin/bash
# Let's broaden our search to find any CI/CD configurations and workflow files
# that might reference linting commands
# Check for any lint-related commands in CI configs
rg -g '*.{yml,yaml}' -i 'lint|eslint' .github/
# Also check for any pnpm commands in CI configs
rg -g '*.{yml,yaml}' 'pnpm' .github/
# Let's also check the root package.json to see the full scripts configuration
cat package.json
Length of output: 2194
packages/cli/src/cli/commands/start.ts (2)
1-24: LGTM: Well-structured command definition.
The command structure is clean with appropriate imports and well-defined arguments.
57-59: Process management improvements needed.
The current implementation lacks proper error handling and cleanup for the child process.
packages/indexer/package.json (3)
74-74: LGTM: test:ci command retained.
The test:ci command is appropriately configured to run tests in CI environment.
Line range hint 13-66: Verify runtime module accessibility.
The exports configuration properly isolates different modules, but given that the PR objectives mention utilizing Rollup's virtual plugin for indexers at build time, we should verify that the necessary runtime modules are properly exposed.
Let's check the runtime module configuration:
#!/bin/bash
# Description: Verify runtime module configuration
# Test 1: Look for runtime-related imports in the codebase
echo "Checking runtime module usage..."
rg -g '!package*.json' "from.*@apibara/indexer/.*runtime"
# Test 2: Check for Rollup configuration files that might need updating
echo "Checking Rollup configuration..."
fd "rollup.config|vite.config"71-71: Verify Biome linter configuration and migration.
The linting tool has been changed to Biome. While this is a good choice for its speed and features, we should ensure proper migration.
Let's verify the Biome setup:
✅ Verification successful
Let me gather more information to verify the Biome setup properly.
Let me check the content of the Biome configuration to ensure it's properly set up.
Biome linter setup is properly configured
The migration to Biome appears to be complete and well-configured:
- A root-level
biome.jsonexists with comprehensive configuration - No conflicting ESLint/Prettier configs found in the repository
- The configuration includes proper rules, formatting options, and ignore patterns
- The lint script in
packages/indexer/package.jsoncorrectly uses the Biome CLI
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Biome configuration and rules
# Test 1: Check if Biome config exists
echo "Checking for Biome configuration..."
fd "biome.json|.biome.json"
# Test 2: Check for any remaining old linter configs that should be removed
echo "Checking for potential conflicting linter configs..."
fd ".eslintrc|.prettierrc"
Length of output: 242
Script:
#!/bin/bash
# Let's check the root directory for Biome configuration
echo "Checking root directory for Biome config..."
ls -la biome.json .biome.json 2>/dev/null || true
# Check package-specific Biome config
echo -e "\nChecking package-specific Biome config..."
ls -la packages/indexer/biome.json packages/indexer/.biome.json 2>/dev/null || true
# Look for any ESLint/Prettier related dependencies in package.json
echo -e "\nChecking for old linter dependencies..."
grep -E "eslint|prettier" packages/indexer/package.json || true
# Check for any existing Biome dependencies
echo -e "\nChecking for Biome dependencies..."
grep -E "@biomejs|biome" packages/indexer/package.json || true
Length of output: 697
Script:
#!/bin/bash
# Check the content of root Biome config
echo "Content of root biome.json:"
cat biome.json
# Check if there are any remaining ESLint/Prettier config files in the monorepo
echo -e "\nChecking for any remaining linter config files in the entire repo..."
fd -H "\.eslintrc|\.prettierrc|\.eslintignore|\.prettierignore" --no-ignore
Length of output: 1255
packages/cli/package.json (2)
73-73: Consider updating esbuild to latest version.
The dependency changes align well with the PR objectives:
- Moving
@apibara/starknetto devDependencies is appropriate if it's only needed for development - Adding Rollup plugins supports the new build-time indexer list generation
However, [email protected] is not the latest version. Consider updating unless there's a specific reason for this version.
Let's check for potential compatibility issues with the latest esbuild:
#!/bin/bash
# Description: Check esbuild usage and compatibility
# Look for esbuild-specific configuration or usage
rg "esbuild" --type tsAlso applies to: 89-89, 103-103
66-66: Document environment variables used in scripts.
The script changes look good and align with the command refactoring. Consider documenting the purpose of environment variables like JITI_ESM_RESOLVE=1 in the README or developer documentation.
Let's check if these variables are documented:
Also applies to: 70-70
| if (!fse.existsSync(entry)) { | ||
| apibara.logger.error( | ||
| `Output directory ${outputDir} does not exist. Try building the indexer with "apibara build" first.`, | ||
| ); | ||
| return process.exit(1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error messaging for missing build.
As discussed in the PR comments, the error message could be improved to match the Next.js style and point to documentation.
if (!fse.existsSync(entry)) {
apibara.logger.error(
- `Output directory ${outputDir} does not exist. Try building the indexer with "apibara build" first.`,
+ [
+ `Could not find a production build in the '${outputDir}' directory.`,
+ `Try building your app with 'apibara build' before starting the indexer.`,
+ `https://docs.apibara.com/errors/no-production-build`
+ ].join('\n')
);
return process.exit(1);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!fse.existsSync(entry)) { | |
| apibara.logger.error( | |
| `Output directory ${outputDir} does not exist. Try building the indexer with "apibara build" first.`, | |
| ); | |
| return process.exit(1); | |
| } | |
| if (!fse.existsSync(entry)) { | |
| apibara.logger.error( | |
| [ | |
| `Could not find a production build in the '${outputDir}' directory.`, | |
| `Try building your app with 'apibara build' before starting the indexer.`, | |
| `https://docs.apibara.com/errors/no-production-build` | |
| ].join('\n') | |
| ); | |
| return process.exit(1); | |
| } |
| "./runtime": { | ||
| "types": "./dist/runtime/index.d.ts", | ||
| "import": "./dist/runtime/index.mjs" | ||
| }, | ||
| "./internal/citty": { | ||
| "import": "./dist/internal/citty/index.mjs", | ||
| "types": "./dist/internal/citty/index.d.ts" | ||
| "./runtime/meta": { | ||
| "types": "./runtime-meta.d.ts", | ||
| "import": "./runtime-meta.mjs" | ||
| }, | ||
| "./runtime/*": { | ||
| "types": "./dist/runtime/*.d.ts", | ||
| "import": "./dist/runtime/*.mjs" | ||
| }, | ||
| "./dist/runtime/*": { | ||
| "types": "./dist/runtime/*.d.ts", | ||
| "import": "./dist/runtime/*.mjs" |
There was a problem hiding this comment.
💡 Codebase verification
Runtime exports configuration needs adjustment
The exports configuration in package.json references files in the dist directory, but the source files are currently only present in src/runtime. The runtime-meta.* files are correctly placed at the package root, but:
./dist/runtime/*.d.tsand./dist/runtime/*.mjspaths don't exist yet- The source files exist at:
src/runtime/dev.tssrc/runtime/index.tssrc/runtime/start.tssrc/runtime/internal/app.ts
Either:
- The build process needs to be set up to generate the dist files, or
- The exports configuration needs to be updated to match the current structure
🔗 Analysis chain
LGTM! Verify runtime directory structure.
The new runtime-related exports are well-structured and consistent with the existing pattern. The wildcard exports will enable flexible runtime module loading as needed for the command refactoring.
Let's verify the runtime directory structure matches the exports configuration:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify runtime directory structure exists as expected
# Check if runtime directory and type definitions exist
fd -t d "runtime$" "packages/cli/(src|dist)"
fd -t f ".*\.d\.ts$" -p "packages/cli/dist/runtime"
Length of output: 322
Script:
#!/bin/bash
# Let's try a different approach to verify the runtime-related files and structure
# Check src directory structure
fd -t d "runtime" "packages/cli/src"
# Check for runtime-related source files
fd -t f ".*\.(ts|js|mjs)$" "packages/cli/src/runtime"
# Check for runtime meta files
fd "runtime-meta" "packages/cli"
# List all files in packages/cli to understand the structure
ls -la packages/cli/
Length of output: 1103
fracek
force-pushed
the
cli-rollup-refactor
branch
from
6c72c6a to
c05f2e5
Compare
fracek
force-pushed
the
cli-rollup-refactor
branch
from
c05f2e5 to
a84b1f5
Compare
fracek
force-pushed
the
cli-rollup-refactor
branch
from
a84b1f5 to
c4c53a8
Compare
fracek
force-pushed
the
cli-rollup-refactor
branch
from
899c633 to
44326f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (12)
examples/cli-js/apibara.config.mjs (2)
4-7: Add documentation for runtime configuration values.The purpose and expected values for
testandcheckconfiguration options are not documented. Consider:
- Adding JSDoc comments to explain each configuration option
- Using TypeScript or providing type information through JSDoc if possible
Example improvement:
export default defineConfig({ + /** Runtime configuration options */ runtimeConfig: { + /** @type {number} Test configuration value */ test: 123, + /** @type {string} Check configuration value */ check: "something", },
1-15: Consider adding runtime configuration validation.Since this configuration file defines runtime values that will be used by the CLI commands, consider:
- Adding runtime validation to ensure required values are present
- Implementing type checking for configuration values
- Adding schema validation to catch configuration errors early
This would help prevent runtime errors and provide better error messages to users.
examples/cli-js/indexers/starknet.indexer.js (1)
5-20: Consider environment configuration and error handling improvements.While the basic setup is functional, there are several areas that could be improved:
- The
streamUrland contractaddressare hardcoded. Consider moving these to environment variables or configuration files for better maintainability and environment flexibility.- The starting block number (800,000) should be documented with a comment explaining its significance.
- Consider adding validation for the contract address format.
Here's a suggested improvement:
+import { config } from 'dotenv'; + +// Load environment variables +config(); + +// Validate contract address +const CONTRACT_ADDRESS = process.env.STARKNET_CONTRACT_ADDRESS || + "0x049d36570d4e46f48e99674bd3fcc84644ddd6b96f7c741b1562b82f9e004dc7"; +if (!/^0x[0-9a-fA-F]{64}$/.test(CONTRACT_ADDRESS)) { + throw new Error('Invalid contract address format'); +} + export default defineIndexer(StarknetStream)({ - streamUrl: "https://starknet.preview.apibara.org", + streamUrl: process.env.STARKNET_STREAM_URL || "https://starknet.preview.apibara.org", finality: "accepted", startingCursor: { + // Start indexing from block 800,000 (Goerli testnet deployment block) orderKey: 800_000n, }, filter: { events: [ { - address: - "0x049d36570d4e46f48e99674bd3fcc84644ddd6b96f7c741b1562b82f9e004dc7", + address: CONTRACT_ADDRESS, keys: [hash.getSelectorFromName("Transfer")], includeReceipt: true, }, ], },.github/workflows/build.yml (1)
46-46: Consider adding a comment for test exclusions.While the build step has a helpful comment explaining the exclusions, the test step would benefit from similar documentation.
- run: pnpm test:ci --filter=!./examples/cli --filter=!./examples/cli-js + # Exclude examples/cli since it requires the cli to be built. + run: pnpm test:ci --filter=!./examples/cli --filter=!./examples/cli-jspackages/cli/src/runtime/dev.ts (3)
6-20: Enhance argument definitions with validation and defaultsConsider improving the argument definitions for better user experience and validation:
args: { indexers: { type: "string", - description: "Which indexers to run", + description: "Comma-separated list of indexers to run (e.g., 'indexer1,indexer2')", + default: undefined, }, preset: { type: "string", - description: "Preset to use", + description: "Configuration preset name to apply", + required: false, + default: "default", }, },
52-60: Enhance CLI metadata descriptionThe current description could be more informative about the development mode capabilities and use cases.
meta: { name: "indexer-dev-runner", - description: "Run indexer in dev mode", + description: "Development environment runner for Apibara indexers with hot-reload support", },
64-64: Remove unnecessary default exportThe empty default export serves no purpose and can be removed.
-export default {};packages/cli/src/rollup/config.ts (2)
Line range hint
24-54: Consider extracting the RollupConfig type extension.While the inline type extension
RollupConfig & { plugins: OutputPluginOption[] }works, it would be cleaner to define this as a separate type.+type ExtendedRollupConfig = RollupConfig & { plugins: OutputPluginOption[] }; + - const rollupConfig: RollupConfig & { plugins: OutputPluginOption[] } = { + const rollupConfig: ExtendedRollupConfig = {
Line range hint
13-79: Well-structured refactor using virtual plugins.The refactoring improves the codebase by:
- Using Rollup's virtual plugins for dynamic content generation
- Maintaining a clear separation of concerns between build configuration and plugin logic
- Providing better type safety through explicit typing
This approach will make it easier to maintain and extend the build process in the future.
packages/cli/package.json (1)
66-66: Consider making the indexer parameter configurable.The
playground:startscript now hardcodes the starknet indexer. Consider making this configurable through an environment variable to maintain flexibility.- "playground:start": "pnpm playground start --dir playground --indexer starknet" + "playground:start": "pnpm playground start --dir playground --indexer ${INDEXER:-starknet}"Also applies to: 70-70
packages/cli/src/runtime/internal/app.ts (2)
18-20: Enhance error message to list available presetsWhen an invalid preset is specified, consider including the list of available presets in the error message to assist users.
Apply this diff to improve the error message:
if (config.presets[preset] === undefined) { throw new Error( - `Specified preset "${preset}" but it was not defined` + `Specified preset "${preset}" is not defined. Available presets are: ${Object.keys(config.presets).join(', ')}` ); }
31-33: Include available indexers in the error messageIf an invalid indexer name is provided, enhance the error message by listing the available indexers to guide users.
Apply this diff to improve the error message:
if (indexerDefinition === undefined) { throw new Error( - `Specified indexer "${indexerName}" but it was not defined` + `Specified indexer "${indexerName}" is not defined. Available indexers are: ${availableIndexers.join(', ')}` ); }
📜 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 (52)
.github/workflows/build.yml(1 hunks)change/@apibara-indexer-843e6831-307c-49b1-bf59-833803a4a27f.json(1 hunks)change/apibara-ed31dbbf-efbd-44c6-8eb0-f729dde0acb6.json(1 hunks)examples/cli-js/apibara.config.mjs(1 hunks)examples/cli-js/indexers/starknet.indexer.js(1 hunks)examples/cli-js/package.json(1 hunks)examples/cli/apibara.config.ts(0 hunks)examples/cli/indexers/starknet.indexer.ts(1 hunks)examples/cli/package.json(1 hunks)flake.nix(1 hunks)package.json(1 hunks)packages/cli/build.config.ts(2 hunks)packages/cli/package.json(4 hunks)packages/cli/playground/apibara.config.ts(1 hunks)packages/cli/playground/indexers/starknet.indexer.ts(2 hunks)packages/cli/playground/tsconfig.json(0 hunks)packages/cli/runtime-meta.d.ts(1 hunks)packages/cli/runtime-meta.mjs(1 hunks)packages/cli/src/cli/commands/build.ts(2 hunks)packages/cli/src/cli/commands/dev.ts(5 hunks)packages/cli/src/cli/commands/prepare.ts(0 hunks)packages/cli/src/cli/commands/start.ts(1 hunks)packages/cli/src/cli/index.ts(1 hunks)packages/cli/src/config/index.ts(1 hunks)packages/cli/src/core/apibara.ts(3 hunks)packages/cli/src/core/build/build.ts(1 hunks)packages/cli/src/core/build/dev.ts(1 hunks)packages/cli/src/core/build/prepare.ts(1 hunks)packages/cli/src/core/build/prod.ts(2 hunks)packages/cli/src/core/build/types.ts(2 hunks)packages/cli/src/core/config/defaults.ts(1 hunks)packages/cli/src/core/config/loader.ts(1 hunks)packages/cli/src/core/config/resolvers/runtime-config.resolver.ts(1 hunks)packages/cli/src/core/config/update.ts(1 hunks)packages/cli/src/core/path.ts(1 hunks)packages/cli/src/core/scan.ts(1 hunks)packages/cli/src/internal/citty/index.ts(0 hunks)packages/cli/src/internal/consola/index.ts(0 hunks)packages/cli/src/rollup/config.ts(3 hunks)packages/cli/src/rollup/plugins/config.ts(1 hunks)packages/cli/src/rollup/plugins/indexers.ts(1 hunks)packages/cli/src/runtime/dev.ts(1 hunks)packages/cli/src/runtime/index.ts(1 hunks)packages/cli/src/runtime/internal/app.ts(1 hunks)packages/cli/src/runtime/start.ts(1 hunks)packages/cli/src/types/apibara.ts(1 hunks)packages/cli/src/types/config.ts(3 hunks)packages/cli/src/types/hooks.ts(1 hunks)packages/cli/src/types/virtual/config.d.ts(1 hunks)packages/cli/src/types/virtual/indexers.d.ts(1 hunks)packages/cli/tsconfig.json(1 hunks)packages/indexer/package.json(1 hunks)
💤 Files with no reviewable changes (5)
- examples/cli/apibara.config.ts
- packages/cli/playground/tsconfig.json
- packages/cli/src/cli/commands/prepare.ts
- packages/cli/src/internal/citty/index.ts
- packages/cli/src/internal/consola/index.ts
✅ Files skipped from review due to trivial changes (3)
- change/@apibara-indexer-843e6831-307c-49b1-bf59-833803a4a27f.json
- examples/cli-js/package.json
- packages/cli/runtime-meta.d.ts
🚧 Files skipped from review as they are similar to previous changes (36)
- change/apibara-ed31dbbf-efbd-44c6-8eb0-f729dde0acb6.json
- examples/cli/indexers/starknet.indexer.ts
- examples/cli/package.json
- flake.nix
- package.json
- packages/cli/playground/apibara.config.ts
- packages/cli/playground/indexers/starknet.indexer.ts
- packages/cli/runtime-meta.mjs
- packages/cli/src/cli/commands/build.ts
- packages/cli/src/cli/commands/dev.ts
- packages/cli/src/cli/commands/start.ts
- packages/cli/src/cli/index.ts
- packages/cli/src/config/index.ts
- packages/cli/src/core/apibara.ts
- packages/cli/src/core/build/build.ts
- packages/cli/src/core/build/dev.ts
- packages/cli/src/core/build/prepare.ts
- packages/cli/src/core/build/prod.ts
- packages/cli/src/core/build/types.ts
- packages/cli/src/core/config/defaults.ts
- packages/cli/src/core/config/loader.ts
- packages/cli/src/core/config/resolvers/runtime-config.resolver.ts
- packages/cli/src/core/config/update.ts
- packages/cli/src/core/path.ts
- packages/cli/src/core/scan.ts
- packages/cli/src/rollup/plugins/config.ts
- packages/cli/src/rollup/plugins/indexers.ts
- packages/cli/src/runtime/index.ts
- packages/cli/src/runtime/start.ts
- packages/cli/src/types/apibara.ts
- packages/cli/src/types/config.ts
- packages/cli/src/types/hooks.ts
- packages/cli/src/types/virtual/config.d.ts
- packages/cli/src/types/virtual/indexers.d.ts
- packages/cli/tsconfig.json
- packages/indexer/package.json
🔇 Additional comments (15)
examples/cli-js/apibara.config.mjs (2)
1-3: LGTM! Good use of ES modules and configuration helper.
The use of defineConfig from apibara/config is a good practice as it likely provides type checking and validation for the configuration object.
8-14: Verify and document preset usage.
While the preset structure allows for environment-specific configurations, it's not clear:
- When and how these presets are applied
- How this relates to the build, dev, and start commands mentioned in the PR objectives
Let's check how presets are used in the codebase:
✅ Verification successful
Presets are used to override configuration at runtime via CLI commands
The verification reveals that presets are properly integrated into the CLI commands:
- Presets can be applied via the
--presetflag in bothdevandstartcommands - When a preset is specified, the configuration is merged using
defuwith preset values taking precedence - In development mode, preset changes trigger a reload of the configuration
The preset in your configuration will be accessible when:
- Running
apibara dev --preset dev - Running
apibara start --indexer <name> --preset dev
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for preset usage in command implementations
rg -l "preset" | grep -i "command\|cli"
# Search for documentation about presets
fd -g "*.md" -x grep -l "preset" {} \;
Length of output: 502
Script:
#!/bin/bash
# Check preset implementation in the config loader and resolver
cat packages/cli/src/core/config/resolvers/preset.resolver.ts
echo "---"
# Check how presets are used in dev and start commands
cat packages/cli/src/cli/commands/dev.ts
echo "---"
cat packages/cli/src/cli/commands/start.ts
Length of output: 5327
examples/cli-js/indexers/starknet.indexer.js (2)
1-3: LGTM: Imports are appropriate and necessary.
The imports are correctly structured and include all required dependencies for the indexer functionality.
17-17: Review performance impact of includeReceipt.
Setting includeReceipt: true will fetch additional data for each event, which might impact performance when processing many events.
Let's check if there are any performance considerations in the documentation or related files:
✅ Verification successful
The previous results show that includeReceipt is used in multiple indexers and tests, but we need to understand its implementation and documentation to assess its performance impact. Let's check the interface definition and surrounding documentation.
Based on the documentation and code analysis, I can now provide a final response:
Setting includeReceipt: true has minimal performance impact
The includeReceipt option is a standard filter parameter that only includes the transaction receipt when retrieving events. From the documentation:
- It defaults to
false, making it an opt-in feature - It only fetches the receipt for the specific transaction that emitted the matched event
- It's commonly used alongside other similar options like
includeTransactionandincludeEvents
Since this is in an example indexer and the receipt data is necessary for processing Transfer events, the performance overhead is justified and expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for documentation or examples related to includeReceipt usage
rg -l "includeReceipt" | xargs rg -A 5 -B 5 "includeReceipt.*true"
# Search for performance-related documentation
rg -i "performance|optimization" README.md docs/
Length of output: 8026
Script:
#!/bin/bash
# Let's check the filter interface and any documentation about includeReceipt
ast-grep --pattern 'interface $_ {
$$$
includeReceipt$_
$$$
}'
# Let's also check if there are any comments or documentation about performance
rg -B 5 -A 5 "includeReceipt" packages/starknet/src/
Length of output: 26280
packages/cli/build.config.ts (4)
5-13: LGTM: Module array updated correctly.
The addition of the "runtime" module is consistent with the PR objectives and follows the existing naming convention.
40-43: LGTM: Externals configuration is properly updated.
The externals array correctly includes the new runtime meta module while preserving the existing module externals. The ordering ensures proper module resolution.
31-38: Verify runtime-meta.mjs file existence and location.
The alias configuration points to runtime-meta.mjs in the parent directory. This is a critical dependency that needs to be verified.
✅ Verification successful
Runtime meta file exists and is correctly configured
The runtime-meta.mjs file exists in the expected location and exports the necessary path configurations for runtime functionality. The alias configuration in build.config.ts correctly points to this file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify runtime-meta.mjs file existence and content
# Check if the runtime-meta.mjs file exists
if [ -f "packages/cli/runtime-meta.mjs" ]; then
echo "runtime-meta.mjs exists. Contents:"
cat packages/cli/runtime-meta.mjs
else
echo "runtime-meta.mjs not found!"
fi
Length of output: 453
26-26: Verify runtime module directory structure.
The runtime module uses a directory input (./src/runtime/) unlike other entries that specify individual files. Please ensure the directory structure exists and contains the necessary files.
✅ Verification successful
The previous script didn't provide clear output, so let's try a different approach to verify the runtime directory structure and its references in the codebase. This will help us understand if the directory input configuration is valid and necessary.
Runtime directory structure is valid and appropriate for directory-based entry
The runtime module directory structure exists and contains multiple TypeScript files (dev.ts, index.ts, start.ts) and an internal subdirectory, justifying the directory-based build entry configuration. The ESM format is also appropriate as this appears to be a modern JavaScript/TypeScript module structure.
packages/cli/src/runtime/dev.tsindex.tsstart.tsinternal/app.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the runtime module directory structure
# Check if the runtime directory exists and list its contents
if [ -d "packages/cli/src/runtime" ]; then
echo "Runtime directory exists. Contents:"
ls -la packages/cli/src/runtime/
else
echo "Runtime directory not found!"
fi
Length of output: 453
Script:
#!/bin/bash
# Let's verify the runtime directory structure using fd and check its contents
fd . packages/cli/src/runtime
# Also check if there are any references to runtime directory in the codebase
rg -l "runtime" packages/cli/src
Length of output: 650
.github/workflows/build.yml (2)
41-42: LGTM! Clear explanation for build exclusions.
The comment clearly explains why examples/cli is excluded from the build process, which helps future maintainers understand the dependency relationship.
41-46: Verify the impact of excluding these directories.
The exclusion of examples/cli and examples/cli-js from both build and test processes seems intentional based on the PR objectives of refactoring CLI commands. However, let's verify that these examples are tested elsewhere or have their own CI workflows.
✅ Verification successful
Let me gather more specific information about these CLI examples to understand their testing setup better.
Based on the gathered information, I can now provide a final response:
The exclusion of CLI examples from build and test processes is correct and intentional
The examples/cli and examples/cli-js directories are demonstration projects that depend on the main CLI package being built first. This is evident from their package.json files which:
- Are marked as
"private": true - Have
apibaraas a workspace dependency - Only contain build/dev/start scripts that use the
apibaraCLI tool - Don't contain any test scripts
These examples are also explicitly ignored in the root package.json's beachball configuration:
"beachball": {
"ignorePatterns": [
"examples/**"
]
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if examples/cli and examples/cli-js have their own workflows
# or are tested in a different way
# Look for other workflow files that might handle these examples
fd -e yml -e yaml . .github/workflows -x cat {} \; | grep -A 5 "examples/cli"
# Check if these directories have their own test scripts
fd package.json -e json . examples/cli examples/cli-js -x jq -r '.scripts | select(.test != null)'
Length of output: 2113
Script:
#!/bin/bash
# Check the package.json files specifically in the CLI example directories
fd package.json . examples/cli examples/cli-js -x cat {}
# Look for any README or documentation that might explain the testing strategy
fd README.md . examples/cli examples/cli-js -x cat {}
# Check if these are integration test examples that might be intentionally excluded
rg -A 5 "examples/cli" . --type md --type txt
Length of output: 23069
packages/cli/src/runtime/dev.ts (2)
1-5: LGTM! Dependencies are well-organized
The imports are clean and necessary for the functionality being implemented.
1-64: Verify CLI integration and type definitions
Please ensure:
- The dev runner commands are properly registered in the main CLI configuration
- The indexer type definitions include the cleanup method for graceful shutdown
packages/cli/src/rollup/config.ts (1)
6-13: LGTM! Clean import organization and simplified function signature.
The changes improve the code by:
- Using
pathefor cross-platform path handling - Adding necessary imports for the new plugin system
- Simplifying the function signature by removing the unused
devparameter
packages/cli/package.json (2)
35-49: Verify virtual plugin configuration for runtime exports.
The runtime exports configuration aligns with the PR objective of using Rollup's virtual plugin. However, we should verify that the virtual plugin is properly configured to generate the expected files.
#!/bin/bash
# Description: Verify virtual plugin configuration in build setup
# Check for virtual plugin configuration in build files
rg -A 5 "@rollup/plugin-virtual" "packages/cli/src"
# Check if runtime meta files are generated
fd "runtime-meta" "packages/cli"73-73: LGTM! Dependencies are well-structured.
The dependency changes align well with the PR objectives:
- Moving
@apibara/starknetto devDependencies is appropriate - Adding rollup plugins supports the new build-time indexer generation
Also applies to: 89-89, 103-103
| async transform({ block: { header } }) { | ||
| console.log("Transforming block ", header?.blockNumber); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance transform function with structured logging and error handling.
The current transform function uses basic console.log and lacks error handling. Consider using a structured logger and adding proper error handling.
Here's a suggested improvement:
- async transform({ block: { header } }) {
- console.log("Transforming block ", header?.blockNumber);
+ async transform({ block: { header, events } }) {
+ try {
+ const blockNumber = header?.blockNumber;
+ if (!blockNumber) {
+ throw new Error('Block number is undefined');
+ }
+
+ console.log({
+ level: 'info',
+ message: 'Transforming block',
+ blockNumber: blockNumber.toString(),
+ eventsCount: events?.length ?? 0,
+ });
+
+ // Process events here
+ } catch (error) {
+ console.error({
+ level: 'error',
+ message: 'Failed to transform block',
+ error: error.message,
+ blockNumber: header?.blockNumber?.toString(),
+ });
+ throw error; // Re-throw to ensure the indexer handles the error
+ }
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async transform({ block: { header } }) { | |
| console.log("Transforming block ", header?.blockNumber); | |
| }, | |
| async transform({ block: { header, events } }) { | |
| try { | |
| const blockNumber = header?.blockNumber; | |
| if (!blockNumber) { | |
| throw new Error('Block number is undefined'); | |
| } | |
| console.log({ | |
| level: 'info', | |
| message: 'Transforming block', | |
| blockNumber: blockNumber.toString(), | |
| eventsCount: events?.length ?? 0, | |
| }); | |
| // Process events here | |
| } catch (error) { | |
| console.error({ | |
| level: 'error', | |
| message: 'Failed to transform block', | |
| error: error.message, | |
| blockNumber: header?.blockNumber?.toString(), | |
| }); | |
| throw error; // Re-throw to ensure the indexer handles the error | |
| } | |
| }, |
| async run({ args }) { | ||
| const { indexers: indexersArgs, preset } = args; | ||
|
|
||
| let selectedIndexers = availableIndexers; | ||
| if (indexersArgs) { | ||
| selectedIndexers = indexersArgs.split(","); | ||
| } | ||
|
|
||
| for (const indexer of selectedIndexers) { | ||
| if (!availableIndexers.includes(indexer)) { | ||
| throw new Error( | ||
| `Specified indexer "${indexer}" but it was not defined`, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| await Promise.all( | ||
| selectedIndexers.map(async (indexer) => { | ||
| const indexerInstance = createIndexer(indexer, preset); | ||
|
|
||
| const client = createClient( | ||
| indexerInstance.streamConfig, | ||
| indexerInstance.options.streamUrl, | ||
| ); | ||
|
|
||
| await run(client, indexerInstance); | ||
| }), | ||
| ); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add graceful shutdown handling
The command should handle process termination signals (SIGINT, SIGTERM) to ensure indexers can clean up resources properly before shutting down.
async run({ args }) {
const { indexers: indexersArgs, preset } = args;
+ const runningIndexers = new Set();
+
+ // Handle graceful shutdown
+ const cleanup = async () => {
+ console.log('\nShutting down indexers...');
+ // Allow indexers to implement a cleanup method
+ for (const indexer of runningIndexers) {
+ try {
+ await indexer.cleanup?.();
+ } catch (error) {
+ console.error(`Error cleaning up indexer:`, error);
+ }
+ }
+ process.exit(0);
+ };
+
+ process.on('SIGINT', cleanup);
+ process.on('SIGTERM', cleanup);
let selectedIndexers = availableIndexers;
// ... rest of the codeCommittable suggestion skipped: line range outside the PR's diff.
packages/cli/src/rollup/config.ts
Outdated
| rollupConfig.plugins.push(commonjs()); | ||
| rollupConfig.plugins.push(json()); | ||
|
|
||
| rollupConfig.plugins.push( | ||
| nodeResolve({ | ||
| extensions, | ||
| preferBuiltins: true, | ||
| mainFields: ["main"], | ||
| }), | ||
| ); | ||
|
|
||
| rollupConfig.plugins.push( | ||
| esbuild({ | ||
| target: "es2022", | ||
| sourceMap: apibara.options.sourceMap, | ||
| }), | ||
| ); | ||
|
|
||
| rollupConfig.plugins.push(indexers(apibara)); | ||
| rollupConfig.plugins.push(appConfig(apibara)); |
There was a problem hiding this comment.
💡 Codebase verification
Plugin configuration needs attention
The verification revealed some issues that need to be addressed:
- The
rollup-plugin-esbuildis imported but not declared in package.json dependencies - The
appConfigplugin is imported from "./plugins/config" but the file doesn't exist in the codebase - The
indexersplugin is properly implemented using@rollup/plugin-virtual
🔗 Analysis chain
Verify plugin order and dependencies.
The plugin order looks correct, with commonjs and json before nodeResolve. The addition of esbuild, indexers, and appConfig plugins aligns with the PR objectives.
Let's verify the dependencies and plugin usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all required dependencies are properly declared
grep -A 10 '"dependencies"' packages/cli/package.json
grep -A 10 '"devDependencies"' packages/cli/package.json
# Verify the new plugins are properly exported
fd -e ts -e js "plugins/(config|indexers)" packages/cli/src/rollup/
Length of output: 837
Script:
#!/bin/bash
# Let's verify the plugin imports and their existence in the codebase
ast-grep --pattern 'import { $_ } from "@rollup/plugin-$_"'
# Check for the custom plugins implementation
fd -e ts "indexers|appConfig" packages/cli/src/rollup/
cat $(fd -e ts "indexers|appConfig" packages/cli/src/rollup/)
Length of output: 741
Script:
#!/bin/bash
# Let's check for esbuild plugin import and appConfig implementation
rg "import.*esbuild.*from" -A 2
fd -e ts "appConfig" packages/cli/src/rollup/
cat $(fd -e ts "appConfig" packages/cli/src/rollup/)
Length of output: 556
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
examples/cli/indexers/starknet.indexer.ts (2)
10-10: Replace console.log with structured loggingConsider using
apibara.loggerinstead ofconsole.logfor consistent logging across the codebase.- console.log("--> Starknet Indexer Runtime Config: ", runtimeConfig); + apibara.logger.info("starknet indexer runtime config", { config: runtimeConfig });
Line range hint
29-31: Replace console.log in transform functionFor consistency with the project's logging standards, replace console.log with structured logging.
async transform({ block: { header } }) { - console.log("Transforming block ", header?.blockNumber); + apibara.logger.debug("transforming block", { blockNumber: header?.blockNumber }); }packages/cli/src/rollup/plugins/esm-shim.ts (4)
17-17: Refine CommonJS syntax detection for accurate matchingThe regular expression
/__filename|__dirname/may match unintended substrings within other variable names (e.g.,my__dirnameVar). To ensure accurate detection of__filenameand__dirname, use word boundaries:-const CJSyntaxRegex = /__filename|__dirname/; +const CJSyntaxRegex = /\b(__filename|__dirname)\b/;
24-24: Prevent multiple shims from being insertedRelying on
code.includes(ESMShim)to check if the shim has already been inserted may not be reliable, especially if code formatting changes. To ensure the shim isn't inserted multiple times, consider setting a flag or modifying the logic to track insertion more robustly.
46-55: Simplify source mapsourcesContenthandlingManually filtering
sourcesContentfor strings may be unnecessary ifsourceMap.sourcesContentis already correctly structured. You can simplify this section by directly usingsourceMap.sourcesContent:-let sourcesContent: string[] | undefined; -if (Array.isArray(sourceMap.sourcesContent)) { - sourcesContent = []; - for (let i = 0; i < sourceMap.sourcesContent.length; i++) { - const content = sourceMap.sourcesContent[i]; - if (typeof content === "string") { - sourcesContent.push(content); - } - } -} +const sourcesContent = sourceMap.sourcesContent;
4-6: Provide rationale for custom ESM shim implementationThe comment indicates this plugin is an alternative to
@rollup/plugin-esm-shim. Providing additional context on why a custom implementation is necessary—such as specific limitations or issues with the existing plugin—would help maintainers understand the reasoning behind this decision.
📜 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 (8)
examples/cli/indexers/starknet.indexer.ts(1 hunks)examples/cli/package.json(2 hunks)examples/indexer/package.json(1 hunks)packages/cli/package.json(4 hunks)packages/cli/src/rollup/config.ts(2 hunks)packages/cli/src/rollup/plugins/esm-shim.ts(1 hunks)packages/cli/src/types/config.ts(4 hunks)packages/indexer/package.json(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/indexer/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli/src/rollup/config.ts
- packages/indexer/package.json
🔇 Additional comments (11)
examples/cli/package.json (1)
17-17: Consider aligning type definitions version with the main package.
The @types/better-sqlite3 (^7.6.11) and better-sqlite3 (^11.5.0) versions are significantly different. This version mismatch might lead to type definition inconsistencies.
Let's check if this is a known issue:
Consider:
- Verifying if type definitions for version 11.x are available
- If not, document any known type inconsistencies
- Set up a reminder to update types when they become available
Also applies to: 26-26
examples/cli/indexers/starknet.indexer.ts (4)
10-11: Verify the intended use of in-memory database
The current implementation uses an in-memory SQLite database which won't persist data between restarts. If this is an example/testing setup, consider adding a comment to clarify the intention. For production use, consider using a file-based database.
+ // Using in-memory database for example purposes only
+ // For production, consider using a file-based database
const database = new Database(":memory:");18-18: LGTM: SQLite persistence plugin configuration
The SQLite persistence plugin is correctly configured with the database instance.
Line range hint 19-27: LGTM: Event filter configuration
The filter configuration correctly targets Transfer events from the specified contract address.
15-17:
Verify the significant change in startingCursor orderKey
The orderKey has been increased from 80,000n to 800,000n (10x increase). Please verify this change won't skip important historical data.
✅ Verification successful
The orderKey change to 800_000n is consistent with other Starknet indexers
The value of 800_000n is already being used in other Starknet indexers across the codebase:
- examples/cli-js/indexers/starknet.indexer.js
- packages/cli/playground/indexers/starknet.indexer.ts
- examples/starknet-client/src/main.ts
This appears to be a standardization of the starting point for Starknet indexers, updating from the previous value of 80_000n seen in examples/starknet-indexer/src/indexer.ts. The change is intentional and aligned with other implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of startingCursor configuration
# to ensure consistency across the codebase
rg -A 2 "startingCursor.*orderKey" --type typescript
Length of output: 89
Script:
#!/bin/bash
# Search for startingCursor configuration using ripgrep without file type filter
rg -A 2 "startingCursor.*orderKey"
Length of output: 1481
Script:
#!/bin/bash
# Find all TypeScript files and search for startingCursor
fd -e ts | xargs rg -A 2 "startingCursor.*orderKey"
Length of output: 1498
Script:
#!/bin/bash
# Use ast-grep to find object literals containing startingCursor
ast-grep --pattern 'startingCursor: {
orderKey: $_
}'
Length of output: 57
Script:
#!/bin/bash
# Look for any configuration files that might contain orderKey settings
rg -A 2 "orderKey.*[0-9]+"
Length of output: 19410
packages/cli/src/types/config.ts (3)
19-23: LGTM: Improved type safety with explicit empty record type.
The change from {} to Record<string, never> as default type parameter is a good improvement. This makes the type constraints more explicit and provides better type inference in TypeScript.
Also applies to: 42-46
75-75: Verify the implementation of new Rollup configuration options.
The addition of sourceMap and commonJS options enhances build configuration flexibility. However, we should verify their proper implementation in the build process.
Let's check the implementation:
Also applies to: 77-77
✅ Verification successful
Both sourceMap and commonJS options are properly implemented in the build configuration
The verification shows that both options are correctly handled in the build process:
sourceMapis properly implemented inpackages/cli/src/rollup/config.tswhere it's passed directly to the Rollup configuration:sourceMap: apibara.options.sourceMapcommonJSoptions are correctly forwarded in the same file:...apibara.options.commonJS- Source map generation is also properly supported in the ESM shim plugin (
packages/cli/src/rollup/plugins/esm-shim.ts)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Rollup configuration handling
# Check how sourceMap option is used
echo "Checking sourceMap implementation:"
rg -A 5 "sourceMap.*config"
# Check CommonJS configuration handling
echo "Checking commonJS implementation:"
ast-grep --pattern 'commonJS.*options'
Length of output: 310
Script:
#!/bin/bash
# Let's try a broader search for Rollup configuration handling
# Search for sourceMap usage in the codebase
echo "Checking sourceMap usage:"
rg -A 5 "sourceMap"
# Search for commonJS related configuration
echo "Checking commonJS configuration:"
rg -A 5 "commonJS"
# Look for Rollup configuration setup
echo "Checking Rollup configuration setup:"
rg -A 10 "rollup.*config"
# Search for build configuration files
echo "Searching for build configuration files:"
fd -e js -e ts "rollup.config"
Length of output: 3653
64-65: Verify the integration of the new required indexersDir property.
The addition of indexersDir aligns with the PR's objective of managing indexers at build time. However, being a required property means existing configurations need updates.
Let's verify the integration:
✅ Verification successful
The new indexersDir property is properly integrated and has sensible defaults
The property is well integrated into the codebase:
- Has a default value of "indexers" in
defaults.ts - Used correctly in
scan.tsto locate and load indexer files - Properly handles non-existent directories with clear error messages
- Supports both
.indexer.tsand.indexer.jsfile extensions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage and implementation of indexersDir
# Check how indexersDir is used in the codebase
echo "Checking indexersDir usage:"
rg -A 5 "indexersDir"
# Look for potential configuration files that need updates
echo "Checking configuration files:"
fd -e config.ts -e config.js
Length of output: 310
Script:
#!/bin/bash
# Let's try a more comprehensive search
# Check for indexersDir usage across the codebase
echo "Checking indexersDir references:"
rg "indexersDir"
# Check for config type usage and implementations
echo "Checking config type usage:"
rg -l "Config" --type typescript
# Look for test configurations
echo "Checking test configurations:"
fd -e test.ts -e test.js
# Check for configuration examples or defaults
echo "Checking for configuration examples:"
fd "default" -e ts -e js
Length of output: 1671
Script:
#!/bin/bash
# Let's check the implementation details and default configuration
# Check the default configuration implementation
echo "Default configuration content:"
cat packages/cli/src/core/config/defaults.ts
# Check how the config is used in scan.ts
echo "Scan implementation:"
cat packages/cli/src/core/scan.ts
# Look for any configuration examples in documentation
echo "Configuration documentation:"
fd README -e md | xargs cat
Length of output: 6994
packages/cli/package.json (3)
35-49: Skip comment about runtime exports configuration.
A previous review already covers the concerns about runtime exports configuration and dist directory structure.
73-73: LGTM! Dependencies align with the refactoring goals.
The dependency changes support the PR objectives:
- Added Rollup plugins for build-time generation of indexer list
- Correctly moved @apibara/starknet to devDependencies
- Added necessary build tools (magic-string, rollup-plugin-esbuild)
Also applies to: 89-89, 99-99, 104-104
66-66: LGTM! Verify the playground scripts functionality.
The script changes align well with the PR objectives:
- Added debug logging for better development experience
- Updated start command to use the starknet indexer
Let's verify the playground scripts are properly configured:
Also applies to: 70-70
✅ Verification successful
Let me gather more information about the playground script configuration to verify its functionality.
Let me check the playground configuration file and indexers directory content to complete the verification.
Let me check the starknet indexer implementation to complete the verification.
Playground script configuration is properly set up
The verification confirms that:
- The playground directory exists with the required configuration and starknet indexer
- The CLI entry point (
packages/cli/src/cli/index.ts) is correctly referenced in the playground script - Debug level addition (
CONSOLA_LEVEL=debug) will work with the consola logger used in the starknet indexer - The environment setup (
JITI_ESM_RESOLVE=1andNODE_OPTIONS) is appropriate for the ESM-based implementation
The playground script changes are well-aligned with the codebase structure and will provide the intended development experience.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify playground scripts configuration
# Check if the playground directory and indexer exist
# Check for playground directory
fd -t d "playground$" .
# Check for starknet indexer references
rg "starknet.*indexer" -A 3
Length of output: 777
Script:
#!/bin/bash
# Check the playground script implementation and dependencies
# Check playground directory contents
ls -la packages/cli/playground/
# Look for playground script references in source
rg "playground" -A 3 packages/cli/src/
# Check for jiti usage and configuration
rg "jiti" -A 3 packages/cli/src/
Length of output: 337
Script:
#!/bin/bash
# Check playground configuration and indexers
# Check playground config content
cat packages/cli/playground/apibara.config.ts
# Check indexers directory content
ls -la packages/cli/playground/indexers/
# Look for CLI entry point referenced in playground script
cat packages/cli/src/cli/index.ts
Length of output: 1127
Script:
#!/bin/bash
# Check starknet indexer implementation
# Check starknet indexer content
cat packages/cli/playground/indexers/starknet.indexer.ts
# Look for any dev command implementation since it's used in playground
cat packages/cli/src/cli/commands/dev.ts
Length of output: 4458
| "scripts": { | ||
| "build": "apibara build", | ||
| "dev": "apibara dev", | ||
| "start": "apibara start", | ||
| "lint": "biome check .", | ||
| "lint:fix": "pnpm lint --write" | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now generate the final response since I have enough information to verify the documentation of the commands.
Commands need documentation
The new scripts (build, dev, start, lint) are not properly documented. While the CLI commands have basic descriptions in the code, there is no user-facing documentation:
- The
@apibara/clipackage README is empty with just "TODO" placeholders - The CLI help text only provides minimal descriptions
- No documentation found in docs directory
🔗 Analysis chain
LGTM! Verify command documentation.
The new scripts align well with the PR objectives for refactoring the build, dev, and start commands. The addition of linting scripts with Biome is also a good practice for code quality.
Let's verify if these commands are documented:
Let me try another search strategy to verify the documentation of these commands.
Let me try one more search to find CLI command documentation in the source code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these commands are documented in README or CLI help
fd -t f "README.md" -X cat {} | rg -A 5 "(?:build|dev|start) command"
Length of output: 449
Script:
#!/bin/bash
# Search for documentation in different locations
echo "=== Searching in README files ==="
fd -t f "README" -X cat {}
echo -e "\n=== Searching in docs directory ==="
fd -t f . -e md -p docs/ -X cat {}
echo -e "\n=== Searching for CLI help text ==="
ast-grep --pattern 'program
.command($_)
.description($_)'
Length of output: 5760
Script:
#!/bin/bash
# Search for CLI command definitions
echo "=== Searching for command definitions ==="
rg -A 3 "\.command\(['\"](?:build|dev|start)['\"].*\)" --type ts
echo -e "\n=== Checking CLI package for documentation ==="
fd -t f . -p "packages/cli" -X cat {}
Length of output: 44122
| for (const match of code.matchAll(/^import\s.*';$/gm)) { | ||
| if (match.length === 0 || typeof match.index !== "number") { | ||
| continue; | ||
| } | ||
|
|
||
| endIndexOfLastImport = match.index + match[0].length; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance regex to accurately match all import statements
The regular expression used in code.matchAll(/^import\s.*';$/gm) may not match all valid import statements. It misses cases such as:
-
Imports using double quotes:
import something from "module"; -
Imports without semicolons:
import something from 'module' -
Imports spanning multiple lines:
import { something, anotherThing } from 'module';
Consider using a parser like @babel/parser or enhancing the regex to handle these scenarios. An improved regex might be:
-for (const match of code.matchAll(/^import\s.*';$/gm)) {
+for (const match of code.matchAll(/^import\s.*?from\s.*?['"];?$/gm)) {However, using a parser would provide more accuracy and maintainability.
| let endIndexOfLastImport = -1; | ||
|
|
||
| // Find the last import statement and its ending index | ||
| for (const match of code.matchAll(/^import\s.*';$/gm)) { | ||
| if (match.length === 0 || typeof match.index !== "number") { | ||
| continue; | ||
| } | ||
|
|
||
| endIndexOfLastImport = match.index + match[0].length; | ||
| } | ||
|
|
||
| const s = new MagicString(code); | ||
| s.appendRight(endIndexOfLastImport, ESMShim); |
There was a problem hiding this comment.
Handle cases with no import statements to prevent errors
If no import statements are found, endIndexOfLastImport remains -1, leading s.appendRight(-1, ESMShim) to insert the shim at the end of the string or potentially cause an error. To ensure the shim is inserted correctly when there are no import statements, consider checking for this case:
if (endIndexOfLastImport !== -1) {
s.appendRight(endIndexOfLastImport, ESMShim);
+} else {
+ // No imports found; insert shim at the beginning
+ s.prepend(ESMShim);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let endIndexOfLastImport = -1; | |
| // Find the last import statement and its ending index | |
| for (const match of code.matchAll(/^import\s.*';$/gm)) { | |
| if (match.length === 0 || typeof match.index !== "number") { | |
| continue; | |
| } | |
| endIndexOfLastImport = match.index + match[0].length; | |
| } | |
| const s = new MagicString(code); | |
| s.appendRight(endIndexOfLastImport, ESMShim); | |
| let endIndexOfLastImport = -1; | |
| // Find the last import statement and its ending index | |
| for (const match of code.matchAll(/^import\s.*';$/gm)) { | |
| if (match.length === 0 || typeof match.index !== "number") { | |
| continue; | |
| } | |
| endIndexOfLastImport = match.index + match[0].length; | |
| } | |
| const s = new MagicString(code); | |
| if (endIndexOfLastImport !== -1) { | |
| s.appendRight(endIndexOfLastImport, ESMShim); | |
| } else { | |
| // No imports found; insert shim at the beginning | |
| s.prepend(ESMShim); | |
| } |
jaipaljadeja
left a comment
jaipaljadeja
left a comment
There was a problem hiding this comment.
So good! Thank you so much!!
2 participants
Context
This PR refactors how the build, dev, and start commands work.
We leverage the Rollup's virtual plugin to generate a list of indexers to build time.
This indexers are then imported by the dev/start command at runtime.