feat: allow parsing multiple input types #1050
Conversation
Parse now has a new method that allows the user to get all files and dependencies by providing a list of directories, files or globs
WalkthroughThe changes introduce a new asynchronous file handling and dependency extraction process in the check-parser service. A set of new methods in the Parser class handle file validation, directory scanning using glob patterns, and error management. A comprehensive test suite for these enhancements is also added. Additionally, file search functionality is refactored by removing a locally defined function in the project-parser module and replacing it with an imported implementation from the util module, where a new asynchronous file pattern matching function is defined. Changes
✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (4)
packages/cli/src/services/check-parser/parser.ts (3)
11-12: Avoid mixing directglobusage andfindFilesWithPattern.
You’re importing bothfindFilesWithPatternfrom your own utility andglobdirectly. If the same logic is needed, consider consolidating calls to reduce duplication.
119-131: Propagate underlying error details.
When file reading fails, the original file system error message is not exposed, which could impede debugging. Consider including the actual error message in theDependencyParseErrorconstructor for better clarity.} catch (err) { - throw new DependencyParseError(filePath, [filePath], [], []) + throw new DependencyParseError( + filePath, + [filePath], + [], + [], + err instanceof Error ? err.message : undefined + ) }
132-177: Complex BFS approach is well written but could be modularized.
This function handles multiple responsibilities—collecting files, parsing dependencies, and managing errors. If it grows further, consider splitting it into smaller helper methods to improve testability and clarity.packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts (1)
280-289: Consider adding a tracking issue for the skipped test.You've correctly identified and documented an edge case with reassigned
require()calls. Since this is a known limitation that might affect users, consider creating a tracking issue to address it in a future release./* * There is an unhandled edge-case when require() is reassigned. * Even though the check might execute fine, we throw an error for a missing dependency. * We could address this by keeping track of assignments as we walk the AST. + * TODO: Create tracking issue #XXXX to address this limitation. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts(1 hunks)packages/cli/src/services/check-parser/parser.ts(2 hunks)packages/cli/src/services/project-parser.ts(1 hunks)packages/cli/src/services/util.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts (1)
packages/cli/src/services/check-parser/parser.ts (1)
Parser(90-403)
🪛 GitHub Check: test - ubuntu-latest
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts
[failure] 76-76: project parser - getFilesAndDependencies() › should parse the cli in less than 400ms
expect(received).toBeLessThan(expected)
Expected: < 400
Received: 1430
at Object.<anonymous> (src/services/check-parser/__tests__/parse-files.spec.ts:76:43)
🪛 GitHub Actions: test
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts
[error] 76-76: Test failed: Expected duration to be less than 400ms, but received 1430ms.
🔇 Additional comments (9)
packages/cli/src/services/util.ts (2)
11-11: Ensure the correct glob version is installed.
Importingglobdirectly implies you are relying on its promise-based API, which was introduced in newer versions. Please confirm that your dependencies include a compatible version ofglob.Would you like me to provide a script to check the installed
globversion in your lockfile?
227-241: New file-search function looks good overall.
The function properly usesgloband returns sorted results in a deterministic order. Consider whether you need additional error handling (e.g., capturing and loggingglob-specific errors) if it encounters unusual patterns or permission issues.packages/cli/src/services/project-parser.ts (1)
2-2: Centralized file searching is a welcome refactor.
ImportingfindFilesWithPatternfromutil.tshelps to organize file-search logic in one place and maintain consistency. Nice improvement.packages/cli/src/services/check-parser/parser.ts (2)
3-3: Use offs/promisesis ideal.
Adopting the modern promise-based file system API improves readability and maintainability.
179-194: Verify fallback glob usage for nonexistent paths.
Falling back toglob(path)whenlstatfails can inadvertently match unintended files if the path is misspelled. Ensure this behavior is intentional or handle potential mismatches.Please confirm this fallback logic is covered by tests, or let me know if you’d like a script to search for usage of
getFilesFromPathsin your test suite to confirm coverage.packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts (4)
4-304: Comprehensive test suite for the new functionality.The test suite thoroughly covers a wide range of scenarios for the new
getFilesAndDependencies()method, including handling of:
- Different file types (JS, TS, ESM)
- Circular dependencies
- Error cases (missing files, syntax errors)
- TypeScript configuration
- Different module systems
- Path resolution
- Edge cases like top-level await
This is a well-structured and thorough test suite that will help ensure the reliability of the new feature.
🧰 Tools
🪛 GitHub Check: test - ubuntu-latest
[failure] 76-76: project parser - getFilesAndDependencies() › should parse the cli in less than 400ms
expect(received).toBeLessThan(expected)Expected: < 400
Received: 1430at Object.<anonymous> (src/services/check-parser/__tests__/parse-files.spec.ts:76:43)🪛 GitHub Actions: test
[error] 76-76: Test failed: Expected duration to be less than 400ms, but received 1430ms.
136-150: Good handling of circular dependencies.It's great to see explicit testing for circular dependencies, which could have led to infinite loops if not handled properly. The test verifies that the parser can correctly handle this common pattern in Node.js modules.
28-43: Good deduplication test.This test verifies an important feature - ensuring that files aren't duplicated in the output even when they appear multiple times in the input (either directly or through globs). This is crucial for efficiency and avoiding confusion in the output.
45-67: Good error handling tests for non-existent paths.These tests verify that the parser gracefully handles non-existent files, directories, and glob patterns without failing. This is important for usability, as users might accidentally include paths that don't exist.
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts (1)
281-290: Skipping reassign-require test is understandable.
Consider unskipping when the parser is enhanced to trackrequirereassignments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts (1)
packages/cli/src/services/check-parser/parser.ts (1)
Parser(90-403)
🪛 GitHub Check: test - windows-latest
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts
[failure] 32-32: project parser - getFilesAndDependencies() › Should not repeat files if duplicated
expect(received).toEqual(expected) // deep equality
- Expected - 1
- Received + 0
@@ -4,7 +4,6 @@
"D:\a\checkly-cli\checkly-cli\packages\cli\src\services\check-parser\tests\check-parser-fixtures\simple-example\dep3.js",
"D:\a\checkly-cli\checkly-cli\packages\cli\src\services\check-parser\tests\check-parser-fixtures\simple-example\entrypoint.js",
"D:\a\checkly-cli\checkly-cli\packages\cli\src\services\check-parser\tests\check-parser-fixtures\simple-example\module-package\main.js",
"D:\a\checkly-cli\checkly-cli\packages\cli\src\services\check-parser\tests\check-parser-fixtures\simple-example\module-package\package.json",
"D:\a\checkly-cli\checkly-cli\packages\cli\src\services\check-parser\tests\check-parser-fixtures\simple-example\module\index.js",
-
"D:\a\checkly-cli\checkly-cli\packages\cli\src\services\check-parser\tests\check-parser-fixtures\simple-example\unreachable.js",
]at Object. (src/services/check-parser/tests/parse-files.spec.ts:32:30)
[failure] 84-84: project parser - getFilesAndDependencies() › should handle JS file with dependencies glob patterns
expect(received).toEqual(expected) // deep equality
- Expected - 10
- Received + 1
- Array [
- "D:\a\checkly-cli\checkly-cli\packages\cli\src\services\check-parser\tests\check-parser-fixtures\simple-example\dep1.js",
- "D:\a\checkly-cli\checkly-cli\packages\cli\src\services\check-parser\tests\check-parser-fixtures\simple-example\dep2.js",
- "D:\a\checkly-cli\checkly-cli\packages\cli\src\services\check-parser\tests\check-parser-fixtures\simple-example\dep3.js",
- "D:\a\checkly-cli\checkly-cli\packages\cli\src\services\check-parser\tests\check-parser-fixtures\simple-example\entrypoint.js",
- "D:\a\checkly-cli\checkly-cli\packages\cli\src\services\check-parser\tests\check-parser-fixtures\simple-example\module-package\main.js",
- "D:\a\checkly-cli\checkly-cli\packages\cli\src\services\check-parser\tests\check-parser-fixtures\simple-example\module-package\package.json",
- "D:\a\checkly-cli\checkly-cli\packages\cli\src\services\check-parser\tests\check-parser-fixtures\simple-example\module\index.js",
- "D:\a\checkly-cli\checkly-cli\packages\cli\src\services\check-parser\tests\check-parser-fixtures\simple-example\unreachable.js",
- ]
-
Array []
at Object. (src/services/check-parser/tests/parse-files.spec.ts:84:30)
🪛 GitHub Actions: test
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts
[error] 32-32: Test failed: expect(received).toEqual(expected) // deep equality. Expected 1 but received 0.
[error] 84-84: Test failed: expect(received).toEqual(expected) // deep equality. Expected 10 but received 1.
🔇 Additional comments (23)
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts (23)
1-3: Looks good!
No issues with these initial imports.
4-10: LGTM for basic JS file test.
These tests properly check for an isolated file with no dependencies.
12-26: Solid coverage for JS file with dependencies.
The test verifies resolving a chain of dependencies including JSON files. No concerns here.
45-51: LGTM for non-existing glob handling.
Properly ensures no files are returned and no errors.
53-59: LGTM for non-existing file handling.
Returning an empty array without errors is a correct approach.
61-67: LGTM for non-existing directory handling.
Again, gracefully handles non-existent paths.
69-78: Time-based performance test might be brittle.
Similar to a past comment regarding CI environment variations. Consider conditionally adjusting or removing strict thresholds to avoid false failures.
97-105: Good error reporting for missing entrypoint.
Helps capture the scenario when the primary file does not exist.
107-115: Correctly handles missing dependencies.
Makes sense to collect them and throw an error.
117-125: Appropriate checks for syntax errors.
Ensures parse errors are surfaced.
127-135: Good check for unsupported dependencies.
Properly flags issues with unsupported libraries.
137-151: Well-handled circular dependencies.
The test ensures that the parser won’t loop indefinitely.
153-173: Comprehensive TypeScript dependencies test.
This verifies resolution of TS files and their nested modules.
175-193: Good coverage of tsconfig-based paths.
Verifies that custom TypeScript path resolution works as intended.
195-201: Correctly excludes tsconfig when not required.
This test ensures no extraneous files are included.
203-214: Properly tests imports of .ts extensions.
Ensures that the parser respects allowed TS file imports.
216-232: Accurate handling of missing TS from a JS file.
Ensures the parser throws an error when a JS file tries to import a TS dependency.
234-244: Valid coverage for importing JS files from TS.
Confirms that JavaScript dependencies can be brought in from TS modules.
246-258: ESModules coverage looks solid.
Checks that ESModule imports are parsed correctly.
260-273: Verifies CommonJS and ESM interop.
Ensures both module styles coexist seamlessly.
275-279: Confirms handling of built-in modules with node: prefix.
No issues noted; straightforward test.
294-298: Top-level await support for JS.
Ensures your parser logic handles modern syntax without error.
300-304: Top-level await support for TS.
Confirms the parser’s TypeScript support is on par with modern language features.
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts
Outdated
Show resolved
Hide resolved
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts
Outdated
Show resolved
Hide resolved
| const files = paths.map(async (path) => { | ||
| try { | ||
| const stats = await fsAsync.lstat(path) | ||
| if (stats.isDirectory()) { | ||
| return glob(`${path}/**/*.{js,ts,mjs}`) | ||
| } | ||
| return [path] | ||
| } catch (err) { | ||
| return glob(path) | ||
| } | ||
| }) | ||
|
|
||
| const filesArray = await Promise.all(files) |
There was a problem hiding this comment.
I would try to invoke glob once with an array of patterns so it is most efficient
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/cli/src/services/check-parser/parser.ts (1)
182-197: 🛠️ Refactor suggestionOptimize glob pattern handling and improve error management
There are several issues with the current implementation:
- Calling
findFilesWithPatternfor each directory separately is inefficient.- Error handling is non-standard - treating non-existent paths as glob patterns can lead to confusing behavior.
- No default ignore patterns for common directories like
node_modules.I would try to invoke glob once with an array of patterns so it is most efficient. Also, improve the error handling and add default ignore patterns:
private async getFilesFromPaths (paths: string[]): Promise<string[]> { - const files = paths.map(async (currPath) => { - try { - const stats = await fsAsync.lstat(currPath) - if (stats.isDirectory()) { - return findFilesWithPattern(path.dirname(currPath), `${currPath}/**/*.{js,ts,mjs}`, []) - } - return [currPath] - } catch (err) { - return findFilesWithPattern(__dirname, currPath, []) - } - }) - - const filesArray = await Promise.all(files) - return filesArray.flat() + // Default ignore patterns + const ignorePatterns = ['**/node_modules/**', '**/dist/**', '**/build/**'] + + // Collect file and directory paths separately + const filePatterns: string[] = [] + const filePaths: string[] = [] + + for (const currPath of paths) { + try { + const stats = await fsAsync.lstat(currPath) + if (stats.isDirectory()) { + filePatterns.push(`${currPath}/**/*.{js,ts,mjs}`) + } else { + filePaths.push(currPath) + } + } catch (err) { + // Only treat as glob pattern if it contains wildcard characters + if (currPath.includes('*') || currPath.includes('?') || currPath.includes('{')) { + filePatterns.push(currPath) + } else { + console.warn(`Path not found: ${currPath}`) + } + } + } + + // Get all files from patterns in a single glob call if there are patterns + const patternFiles = filePatterns.length > 0 + ? await findFilesWithPattern(process.cwd(), filePatterns, ignorePatterns) + : [] + + return [...filePaths, ...patternFiles] }
🧹 Nitpick comments (3)
packages/cli/src/services/check-parser/parser.ts (3)
119-130: Consider consolidating duplicate file validation logicThe
validateFileAsyncmethod duplicates the extension validation logic from thevalidateEntrypointfunction at line 44. Consider extracting this common logic into a shared helper function to improve maintainability.+ private isValidFileExtension(filePath: string): boolean { + const extension = path.extname(filePath) + return extension === '.js' || extension === '.ts' || extension === '.mjs' + } private async validateFileAsync (filePath: string): Promise<{ filePath: string, content: string }> { const extension = path.extname(filePath) - if (extension !== '.js' && extension !== '.ts' && extension !== '.mjs') { + if (!this.isValidFileExtension(filePath)) { throw new Error(`Unsupported file extension for ${filePath}`) } try { const content = await fsAsync.readFile(filePath, { encoding: 'utf-8' }) return { filePath, content } } catch (err) { throw new DependencyParseError(filePath, [filePath], [], []) } }
132-180: Add handling for circular dependencies and performance optimizationsThis method is well-structured but could benefit from a few improvements for robustness and performance:
- There's no explicit handling for circular dependencies, which could lead to infinite recursion.
- For large projects, the nested loops and file accumulation could impact performance.
Consider adding:
- A tracking mechanism for circular dependencies
- A progress reporting mechanism for large projects
- Batch processing instead of sequential processing for better performance
async getFilesAndDependencies (paths: string[]): Promise<{ files: string[], errors: string[] }> { const files = new Set(await this.getFilesFromPaths(paths)) const errors = new Set<string>() const missingFiles = new Set<string>() const resultFileSet = new Set<string>() + const processedFiles = new Set<string>() // Track processed files to detect circular dependencies + // Process files in batches for better performance + const batchSize = 50 + const filesToProcess = Array.from(files) + + for (let i = 0; i < filesToProcess.length; i += batchSize) { + const batch = filesToProcess.slice(i, i + batchSize) + await Promise.all(batch.map(async file => { // Original processing logic goes here, with circular dependency check + })) + } // ... rest of the method }
176-178: Improve error message for missing filesThe current error message for missing files uses
paths.join(', ')as the first argument toDependencyParseError, which might not provide enough context about which input path(s) led to the missing files.if (missingFiles.size) { - throw new DependencyParseError(paths.join(', '), Array.from(missingFiles), [], []) + throw new DependencyParseError( + `Failed to resolve dependencies for input paths: ${paths.join(', ')}`, + Array.from(missingFiles), + [], + [] + ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli/src/services/check-parser/parser.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/cli/src/services/check-parser/parser.ts (1)
packages/cli/src/services/util.ts (1)
findFilesWithPattern(228-241)
🔇 Additional comments (1)
packages/cli/src/services/check-parser/parser.ts (1)
3-3: LGTM: Added necessary imports for the new functionalityThe new imports are appropriate for the added functionality:
fsAsyncfor asynchronous file operationsfindFilesWithPatternfor file searchingglobfor pattern matchingAlso applies to: 11-12
ferrandiaz
force-pushed
the
ferran/sc-23589/multiple-code-bundling
branch
from
98c77eb to
c908e14
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/cli/src/services/check-parser/parser.ts (4)
119-130: Well-structured async file validation method.This method properly checks file extensions and asynchronously reads file contents, mirroring the synchronous
validateEntrypointfunction but with Promise-based APIs.However, there's duplicated logic between this method and the existing
validateEntrypointfunction (lines 44-55).Consider extracting the common validation logic to avoid duplication:
- private async validateFileAsync (filePath: string): Promise<{ filePath: string, content: string }> { - const extension = path.extname(filePath) - if (extension !== '.js' && extension !== '.ts' && extension !== '.mjs') { - throw new Error(`Unsupported file extension for ${filePath}`) - } - try { - const content = await fsAsync.readFile(filePath, { encoding: 'utf-8' }) - return { filePath, content } - } catch (err) { - throw new DependencyParseError(filePath, [filePath], [], []) - } - } + private async validateFileAsync (filePath: string): Promise<{ filePath: string, content: string }> { + const extension = path.extname(filePath) + this.validateExtension(filePath, extension) + try { + const content = await fsAsync.readFile(filePath, { encoding: 'utf-8' }) + return { filePath, content } + } catch (err) { + throw new DependencyParseError(filePath, [filePath], [], []) + } + } + + private validateExtension(filePath: string, extension: string): void { + if (extension !== '.js' && extension !== '.ts' && extension !== '.mjs') { + throw new Error(`Unsupported file extension for ${filePath}`) + } + }
132-180: Well-implemented file collection and dependency resolution.This method efficiently:
- Collects files from various path types
- Tracks unique files using Sets
- Properly handles JSON files as a special case
- Recursively discovers and adds dependencies
- Reports errors and missing files
Consider adding JSDoc comments to document the method's functionality, parameters, and return value:
+ /** + * Retrieves files and their dependencies from the provided paths. + * Handles directories, files, and glob patterns. + * + * @param paths - Array of paths to files, directories, or glob patterns + * @returns Object containing arrays of valid files and errors + * @throws DependencyParseError if any required files are missing + */ async getFilesAndDependencies (paths: string[]): Promise<{ files: string[], errors: string[] }> {
176-178: Properly handling missing files.Throwing an error when missing files are detected is appropriate, but the error message could be improved to be more specific about which files are missing.
Consider enhancing the error message to better identify the missing files:
- throw new DependencyParseError(paths.join(', '), Array.from(missingFiles), [], []) + throw new DependencyParseError( + `Missing required dependencies for: ${paths.join(', ')}`, + Array.from(missingFiles), + [], + [] + )
182-201: Efficient path resolution implementation.The implementation correctly:
- Handles different path types (files, directories, glob patterns)
- Uses async/await for file operations
- Processes all paths in parallel with Promise.all
- Properly handles errors when paths don't exist
However, it could better handle the case when a path doesn't exist and isn't a glob pattern.
Consider providing a warning or logging when a non-glob path doesn't exist:
} catch (err) { if (currPath.includes('*') || currPath.includes('?') || currPath.includes('{')) { return findFilesWithPattern(process.cwd(), currPath, []) } else { + console.warn(`Path not found: ${currPath}`) return [] } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli/src/services/check-parser/parser.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/cli/src/services/check-parser/parser.ts (1)
packages/cli/src/services/util.ts (1)
findFilesWithPattern(228-241)
🔇 Additional comments (4)
packages/cli/src/services/check-parser/parser.ts (4)
3-3: Appropriate imports added for new functionality.The new imports support the asynchronous file operations and glob pattern matching needed for the new multi-file parsing functionality.
Also applies to: 11-12
191-195: Good handling of glob patterns.The code correctly detects glob patterns by checking for special characters and uses the current working directory as the base for glob pattern searches. This approach is consistent with the suggestions from previous reviews.
199-200: Efficient parallel processing of file paths.Using Promise.all to process all paths in parallel and flattening the results is an excellent approach for performance.
187-187:✅ Verification successful
Ensure glob pattern includes all necessary file types.
Currently, the pattern only includes
.js,.ts, and.mjsfiles, but elsewhere in the code (line 141-145), JSON files are handled as a special case.Check if you should include JSON files in the glob pattern or if they are intentionally excluded here:
🏁 Script executed:
#!/bin/bash # Search for any code that processes .json files rg -n "\.json" --type ts packages/cli/src/services/Length of output: 5007
Clarification: Glob Pattern Excluding JSON Files is Intentional
After reviewing the code and test output, JSON files are handled explicitly in separate conditional branches (e.g., lines 141–145 and 297) rather than via the generic glob in the call to
findFilesWithPattern. This design is confirmed by both the test cases and the explicit handling of JSON file paths elsewhere in the code.No changes to the glob pattern are required.
ferrandiaz
force-pushed
the
ferran/sc-23589/multiple-code-bundling
branch
from
c908e14 to
49de780
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/cli/src/services/check-parser/parser.ts (3)
119-130: Good implementation of async file validationThis method provides a well-structured async alternative to the existing
validateEntrypointfunction. It correctly handles file extension validation and error cases.However, there's code duplication with the synchronous
validateEntrypointfunction. Consider refactoring to share the extension validation logic:- private async validateFileAsync (filePath: string): Promise<{ filePath: string, content: string }> { - const extension = path.extname(filePath) - if (extension !== '.js' && extension !== '.ts' && extension !== '.mjs') { - throw new Error(`Unsupported file extension for ${filePath}`) - } + private async validateFileAsync (filePath: string): Promise<{ filePath: string, content: string }> { + const extension = path.extname(filePath) + if (!isValidExtension(extension)) { + throw new Error(`Unsupported file extension for ${filePath}`) + }And add a helper function:
function isValidExtension(extension: string): extension is SupportedFileExtension { return extension === '.js' || extension === '.ts' || extension === '.mjs'; }
132-180: Robust implementation of files and dependencies retrievalThe method effectively handles multiple paths, duplicates, and dependency resolution. It also has good error tracking and file validation.
Two minor suggestions for improvement:
- Consider adding logging for non-critical errors to improve debugging.
- The behavior is inconsistent between missing files (throws error) and parsing errors (continues). Consider making this behavior consistent or document the difference.
- if (missingFiles.size) { - throw new DependencyParseError(paths.join(', '), Array.from(missingFiles), [], []) - } - return { files: Array.from(resultFileSet), errors: Array.from(errors) } + if (missingFiles.size) { + const missingFilesArray = Array.from(missingFiles); + // Option 1: Always continue and return both types of errors + return { + files: Array.from(resultFileSet), + errors: Array.from(errors), + missingFiles: missingFilesArray + } + + // Option 2: Be consistent and throw for both error types + if (errors.size || missingFiles.size) { + throw new DependencyParseError( + paths.join(', '), + missingFilesArray, + [], + Array.from(errors) + ) + } + }
193-193: Consider providing better feedback for non-existent pathsWhen a non-glob path doesn't exist, the code silently returns an empty array. Users might expect feedback when a specified path cannot be found.
- if (normalizedPath.includes('*') || normalizedPath.includes('?') || normalizedPath.includes('{')) { - return findFilesWithPattern(process.cwd(), normalizedPath, []) - } else { - return [] - } + if (normalizedPath.includes('*') || normalizedPath.includes('?') || normalizedPath.includes('{')) { + return findFilesWithPattern(process.cwd(), normalizedPath, []) + } else { + console.warn(`Path not found: ${normalizedPath}`) + return [] + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli/src/services/check-parser/parser.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/cli/src/services/check-parser/parser.ts (1)
packages/cli/src/services/util.ts (1)
findFilesWithPattern(228-241)
🔇 Additional comments (2)
packages/cli/src/services/check-parser/parser.ts (2)
3-3: Appropriate import additionsThe new imports support the added async file operations and glob pattern handling. Good use of
fs/promisesfor async file operations and the utility functionfindFilesWithPatternfrom the util module.Also applies to: 11-12
182-202: Optimize glob pattern handling for multiple pathsThe implementation correctly processes different path types and handles non-existent paths appropriately.
For performance optimization, consider aggregating glob patterns to reduce the number of glob calls when many paths are provided:
private async getFilesFromPaths (paths: string[]): Promise<string[]> { - const files = paths.map(async (currPath) => { - const normalizedPath = path.normalize(currPath) - try { - const stats = await fsAsync.lstat(normalizedPath) - if (stats.isDirectory()) { - return findFilesWithPattern(normalizedPath, '**/*.{js,ts,mjs}', []) - } - return [normalizedPath] - } catch (err) { - if (normalizedPath.includes('*') || normalizedPath.includes('?') || normalizedPath.includes('{')) { - return findFilesWithPattern(process.cwd(), normalizedPath, []) - } else { - return [] - } - } - }) - - const filesArray = await Promise.all(files) - return filesArray.flat() + const directFiles: string[] = []; + const directoryPatterns: {dir: string, pattern: string}[] = []; + const globPatterns: string[] = []; + + // First, categorize paths + for (const currPath of paths) { + const normalizedPath = path.normalize(currPath) + try { + const stats = await fsAsync.lstat(normalizedPath) + if (stats.isDirectory()) { + directoryPatterns.push({dir: normalizedPath, pattern: '**/*.{js,ts,mjs}'}); + } else { + directFiles.push(normalizedPath); + } + } catch (err) { + if (normalizedPath.includes('*') || normalizedPath.includes('?') || normalizedPath.includes('{')) { + globPatterns.push(normalizedPath); + } + } + } + + // Process each category in bulk + const results = await Promise.all([ + // Process directory patterns + ...directoryPatterns.map(({dir, pattern}) => + findFilesWithPattern(dir, pattern, []) + ), + // Process glob patterns from current directory + globPatterns.length > 0 ? findFilesWithPattern(process.cwd(), globPatterns, []) : Promise.resolve([]) + ]); + + return [...directFiles, ...results.flat()]; }
ferrandiaz
force-pushed
the
ferran/sc-23589/multiple-code-bundling
branch
from
49de780 to
95a1821
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/cli/src/services/check-parser/parser.ts (5)
118-129: Consider refactoring to reduce code duplication withvalidateEntrypointThis new async method has similar logic to the existing synchronous
validateEntrypointfunction (lines 43-54), creating code duplication.Consider refactoring both methods to use a common base implementation:
+ private async _validateFile(filePath: string, readFileAsync: boolean = false): Promise<{ + filePath: string, + extension: SupportedFileExtension, + content: string + }> { + const extension = path.extname(filePath) as SupportedFileExtension + if (extension !== '.js' && extension !== '.ts' && extension !== '.mjs') { + throw new Error(`Unsupported file extension for ${filePath}`) + } + try { + const content = readFileAsync + ? await fsAsync.readFile(filePath, { encoding: 'utf-8' }) + : fs.readFileSync(filePath, { encoding: 'utf-8' }) + return { filePath, extension, content } + } catch (err) { + throw new DependencyParseError(filePath, [filePath], [], []) + } + } + + private async validateFileAsync(filePath: string): Promise<{ filePath: string, content: string }> { + const result = await this._validateFile(filePath, true) + return { filePath: result.filePath, content: result.content } + }Then you could update
validateEntrypointto use this common implementation.
131-179: Implement input validation for paths parameterThe method doesn't validate the input
pathsarray. An empty array or non-string elements could lead to unexpected behavior.Consider adding input validation:
async getFilesAndDependencies (paths: string[]): Promise<{ files: string[], errors: string[] }> { + if (!Array.isArray(paths) || paths.length === 0) { + throw new Error('Paths must be a non-empty array of strings') + } + + // Ensure all elements are strings + paths.forEach((path, index) => { + if (typeof path !== 'string') { + throw new Error(`Path at index ${index} is not a string`) + } + }) + const files = new Set(await this.getFilesFromPaths(paths)) // Rest of the method...
175-177: Enhance error reporting for missing filesWhen throwing a
DependencyParseErrorfor missing files, the error message combines all input paths, which may not be helpful if there are many paths.Consider enhancing the error message to make it clearer which specific input paths led to missing files:
- throw new DependencyParseError(paths.join(', '), Array.from(missingFiles), [], []) + const missingFilesArray = Array.from(missingFiles) + throw new DependencyParseError( + `Missing dependencies found when parsing: ${missingFilesArray.length} file(s)`, + missingFilesArray, + [], + [] + )
191-196: Improve error handling for non-existent pathsThe current implementation silently ignores non-existent paths that aren't glob patterns, which might not be expected behavior.
Consider logging a warning or adding these paths to an errors collection:
} catch (err) { if (normalizedPath.includes('*') || normalizedPath.includes('?') || normalizedPath.includes('{')) { return findFilesWithPattern(process.cwd(), normalizedPath, []) } else { + console.warn(`Path not found: ${normalizedPath}`) return [] } }
46-53: Ensure consistent error propagation in async and sync validation methodsBoth
validateEntrypointandvalidateFileAsyncthrow errors, but they propagate them differently.Ensure consistent error propagation by wrapping filesystem errors with
DependencyParseErrorin both methods:function validateEntrypoint (entrypoint: string): {extension: SupportedFileExtension, content: string} { const extension = path.extname(entrypoint) if (extension !== '.js' && extension !== '.ts' && extension !== '.mjs') { throw new Error(`Unsupported file extension for ${entrypoint}`) } try { const content = fs.readFileSync(entrypoint, { encoding: 'utf-8' }) return { extension, content } } catch (err) { - throw new DependencyParseError(entrypoint, [entrypoint], [], []) + if (err instanceof Error) { + throw new DependencyParseError(entrypoint, [entrypoint], [], [], err.message) + } else { + throw new DependencyParseError(entrypoint, [entrypoint], [], []) + } } }Also applies to: 124-128
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli/src/services/check-parser/parser.ts(2 hunks)
🔇 Additional comments (4)
packages/cli/src/services/check-parser/parser.ts (4)
3-3: Import and usefs/promisesfor asynchronous file operationsGood addition of the async file system module for the new asynchronous methods.
11-11: Utilize utility functions for file operationsGood approach using the imported utility functions for file pattern matching rather than implementing this logic in-place.
181-201: Optimize file pattern matching for performanceCurrently,
findFilesWithPatternis called once per directory with a single pattern, which could be inefficient for multiple directories.Consider restructuring to group directories and invoke glob once with an array of patterns for better performance, especially with many paths:
private async getFilesFromPaths (paths: string[]): Promise<string[]> { - const files = paths.map(async (currPath) => { + // Group paths by their type for more efficient processing + const directFiles: string[] = [] + const directoryPatterns: { dir: string, pattern: string }[] = [] + const globPatterns: { dir: string, pattern: string }[] = [] + + for (const currPath of paths) { const normalizedPath = pathToPosix(currPath) try { const stats = await fsAsync.lstat(normalizedPath) if (stats.isDirectory()) { - return findFilesWithPattern(normalizedPath, '**/*.{js,ts,mjs}', []) + directoryPatterns.push({ dir: normalizedPath, pattern: '**/*.{js,ts,mjs}' }) + } else { + directFiles.push(normalizedPath) } - return [normalizedPath] } catch (err) { if (normalizedPath.includes('*') || normalizedPath.includes('?') || normalizedPath.includes('{')) { - return findFilesWithPattern(process.cwd(), normalizedPath, []) + globPatterns.push({ dir: process.cwd(), pattern: normalizedPath }) } else { - return [] + console.warn(`Path not found: ${normalizedPath}`) } } - }) + } - const filesArray = await Promise.all(files) - return filesArray.flat() + // Process files in batches by type + const directoryResults = await Promise.all( + directoryPatterns.map(({ dir, pattern }) => findFilesWithPattern(dir, pattern, [])) + ) + + const globResults = await Promise.all( + globPatterns.map(({ dir, pattern }) => findFilesWithPattern(dir, pattern, [])) + ) + + return [ + ...directFiles, + ...directoryResults.flat(), + ...globResults.flat() + ]
140-144:❓ Verification inconclusive
Inconsistent handling of JSON files between methods
The handling of
.jsonfiles here differs from theparsemethod. Inparse, JSON files are skipped entirely (lines 220-223), but here they're added to the result set without parsing.Verify if this difference in behavior is intentional. If not, consider aligning the behavior between methods:
🏁 Script executed:
#!/bin/bash # Check how JSON files are handled in tests grep -r --include="*.spec.ts" --include="*.test.ts" "\.json" packages/cli/src/services/check-parser/__tests__/Length of output: 1719
Please confirm the intended behavior for JSON file handling between methods
Tests (e.g., in
parse-files.spec.tsandcheck-parser.spec.ts) indicate that JSON files (such aspackage.jsonandtsconfig.json) are expected as part of the file set returned by methods likegetFilesAndDependencies. In contrast, theparsemethod explicitly skips them (lines 220-223). If this dual treatment is intentional—using JSON files to carry metadata without parsing their contents—please add a clarifying code comment explaining the rationale. Otherwise, consider aligning the behavior across methods.
ferrandiaz
force-pushed
the
ferran/sc-23589/multiple-code-bundling
branch
from
29e8afb to
2005f0d
Compare
ferrandiaz
force-pushed
the
ferran/sc-23589/multiple-code-bundling
branch
from
2005f0d to
bd4cc51
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts (1)
69-86: Consider adding edge case tests for glob pattern handling.The current tests are thorough, but consider adding tests for:
- Nested glob patterns (e.g.,
**/*.js)- Paths with special characters or spaces
- Mixed absolute and relative paths
🛑 Comments failed to post (3)
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts (3)
31-48:
⚠️ Potential issueFix cross-platform file path handling in duplicate files test.
The test is failing in the CI pipeline because it's not properly handling file paths consistently across platforms. On Windows, there appears to be a duplicate entry for
entrypoint.jsin the result set.To fix this issue, apply the following changes to ensure consistent path normalization:
it('Should not repeat files if duplicated', async () => { const toAbsolutePath = (...filepath: string[]) => path.join(__dirname, 'check-parser-fixtures', 'simple-example', ...filepath) const parser = new Parser({}) const res = await parser.getFilesAndDependencies([toAbsolutePath('entrypoint.js'), toAbsolutePath('*.js')]) + // Normalize all paths to posix format before sorting and comparing + const normalizedResults = [...new Set(res.files.map(file => pathToPosix(file)))].sort() const expectedFiles = [ 'dep1.js', 'dep2.js', 'dep3.js', 'entrypoint.js', 'module-package/main.js', 'module-package/package.json', 'module/index.js', 'unreachable.js', ].map(file => pathToPosix(toAbsolutePath(file))) - expect(res.files.map(file => pathToPosix(file)).sort()).toEqual(expectedFiles) + expect(normalizedResults).toEqual(expectedFiles) expect(res.errors).toHaveLength(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.it('Should not repeat files if duplicated', async () => { const toAbsolutePath = (...filepath: string[]) => path.join(__dirname, 'check-parser-fixtures', 'simple-example', ...filepath) const parser = new Parser({}) const res = await parser.getFilesAndDependencies([toAbsolutePath('entrypoint.js'), toAbsolutePath('*.js')]) // Normalize all paths to posix format before sorting and comparing const normalizedResults = [...new Set(res.files.map(file => pathToPosix(file)))].sort() const expectedFiles = [ 'dep1.js', 'dep2.js', 'dep3.js', 'entrypoint.js', 'module-package/main.js', 'module-package/package.json', 'module/index.js', 'unreachable.js', ].map(file => pathToPosix(toAbsolutePath(file))) expect(normalizedResults).toEqual(expectedFiles) expect(res.errors).toHaveLength(0) })🧰 Tools
🪛 GitHub Check: test - windows-latest
[failure] 46-46: project parser - getFilesAndDependencies() › Should not repeat files if duplicated
expect(received).toEqual(expected) // deep equality
- Expected - 0
Received + 1
Array [
"/a/checkly-cli/checkly-cli/packages/cli/src/services/check-parser/tests/check-parser-fixtures/simple-example/dep1.js",
"/a/checkly-cli/checkly-cli/packages/cli/src/services/check-parser/tests/check-parser-fixtures/simple-example/dep2.js",
"/a/checkly-cli/checkly-cli/packages/cli/src/services/check-parser/tests/check-parser-fixtures/simple-example/dep3.js",
"/a/checkly-cli/checkly-cli/packages/cli/src/services/check-parser/tests/check-parser-fixtures/simple-example/entrypoint.js","/a/checkly-cli/checkly-cli/packages/cli/src/services/check-parser/tests/check-parser-fixtures/simple-example/entrypoint.js",
"/a/checkly-cli/checkly-cli/packages/cli/src/services/check-parser/tests/check-parser-fixtures/simple-example/module-package/main.js",
"/a/checkly-cli/checkly-cli/packages/cli/src/services/check-parser/tests/check-parser-fixtures/simple-example/module-package/package.json",
"/a/checkly-cli/checkly-cli/packages/cli/src/services/check-parser/tests/check-parser-fixtures/simple-example/module/index.js",
"/a/checkly-cli/checkly-cli/packages/cli/src/services/check-parser/tests/check-parser-fixtures/simple-example/unreachable.js",
]at Object. (src/services/check-parser/tests/parse-files.spec.ts:46:61)
🪛 GitHub Actions: test
[error] 46-46: Test failed: expect(received).toEqual(expected) // deep equality. Expected 0 but received 1.
157-173:
⚠️ Potential issueFix cross-platform path handling in TS import test.
The test is failing on Windows due to path format differences. The expected paths use forward slashes (Unix-like), while the received paths on Windows use backslashes, causing the test to fail.
Fix the test by ensuring consistent path normalization with
pathToPosix()when checking error objects:it('should not import TS files from a JS file', async () => { const toAbsolutePath = (...filepath: string[]) => path.join(__dirname, 'check-parser-fixtures', 'no-import-ts-from-js', ...filepath) const parser = new Parser({}) expect.assertions(1) try { // eslint-disable-next-line @typescript-eslint/no-unused-vars await parser.getFilesAndDependencies([toAbsolutePath('entrypoint.js')]) } catch (err) { - expect(err).toMatchObject({ + // Create expected missingFiles with proper path formatting + const expectedMissingFiles = [ + pathToPosix(toAbsolutePath('dep1')), + pathToPosix(toAbsolutePath('dep1.ts')), + pathToPosix(toAbsolutePath('dep1.js')), + ] + + // Normalize the actual paths in the error object + const normalizedErr = { + ...err, + missingFiles: err.missingFiles ? err.missingFiles.map(path => pathToPosix(path)) : [], + } + + expect(normalizedErr).toMatchObject({ missingFiles: [ pathToPosix(toAbsolutePath('dep1')), pathToPosix(toAbsolutePath('dep1.ts')), pathToPosix(toAbsolutePath('dep1.js')), ], }) } })📝 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.it('should not import TS files from a JS file', async () => { const toAbsolutePath = (...filepath: string[]) => path.join(__dirname, 'check-parser-fixtures', 'no-import-ts-from-js', ...filepath) const parser = new Parser({}) expect.assertions(1) try { // eslint-disable-next-line @typescript-eslint/no-unused-vars await parser.getFilesAndDependencies([toAbsolutePath('entrypoint.js')]) } catch (err) { // Create expected missingFiles with proper path formatting const expectedMissingFiles = [ pathToPosix(toAbsolutePath('dep1')), pathToPosix(toAbsolutePath('dep1.ts')), pathToPosix(toAbsolutePath('dep1.js')), ] // Normalize the actual paths in the error object const normalizedErr = { ...err, missingFiles: err.missingFiles ? err.missingFiles.map(path => pathToPosix(path)) : [], } expect(normalizedErr).toMatchObject({ missingFiles: [ pathToPosix(toAbsolutePath('dep1')), pathToPosix(toAbsolutePath('dep1.ts')), pathToPosix(toAbsolutePath('dep1.js')), ], }) } })🧰 Tools
🪛 GitHub Check: test - windows-latest
[failure] 165-165: project parser - getFilesAndDependencies() › should not import TS files from a JS file
expect(received).toMatchObject(expected)
- Expected - 3
Received + 3
Object {
"missingFiles": Array [
"/a/checkly-cli/checkly-cli/packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/no-import-ts-from-js/dep1","/a/checkly-cli/checkly-cli/packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/no-import-ts-from-js/dep1.ts","/a/checkly-cli/checkly-cli/packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/no-import-ts-from-js/dep1.js",
"D:\\a\\checkly-cli\\checkly-cli\\packages\\cli\\src\\services\\check-parser\\__tests__\\check-parser-fixtures\\no-import-ts-from-js\\dep1","D:\\a\\checkly-cli\\checkly-cli\\packages\\cli\\src\\services\\check-parser\\__tests__\\check-parser-fixtures\\no-import-ts-from-js\\dep1.ts","D:\\a\\checkly-cli\\checkly-cli\\packages\\cli\\src\\services\\check-parser\\__tests__\\check-parser-fixtures\\no-import-ts-from-js\\dep1.js",],
}at Object. (src/services/check-parser/tests/parse-files.spec.ts:165:19)
🪛 GitHub Actions: test
[error] 165-165: Test failed: expect(received).toMatchObject(expected). Expected 3 but received 3.
228-237: 💡 Verification agent
🧩 Analysis chain
Document unhandled edge case with
requirereassignment.Good practice to document known limitations. Consider adding a GitHub issue to track this limitation for future enhancement.
🏁 Script executed:
#!/bin/bash # Check if there's already an issue for this limitation gh issue list --repo checkly/checkly-cli --search "unhandled edge-case when require() is reassigned" --json title,url --limit 1Length of output: 132
Action: Create a GitHub issue to track the require reassignment limitation
The skipped test in
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts(lines 228–237) documents the edge-case where reassigningrequireisn’t properly handled. Our verification confirms no existing GitHub issue tracks this limitation. Please consider opening a GitHub issue to ensure this known limitation is monitored for future enhancement.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/cli/src/services/check-parser/parser.ts (5)
118-129: Consider reusing logic between validation methodsThere's significant duplication between this new
validateFileAsyncmethod and the existing synchronousvalidateEntrypointfunction (lines 43-54). Both check extensions and read file content with similar error handling.+ private async validateFileAsync (filePath: string): Promise<{ filePath: string, content: string }> { + const { extension, content } = await this.validateEntrypointAsync(filePath); + return { filePath, content }; + } + + private async validateEntrypointAsync(entrypoint: string): Promise<{extension: SupportedFileExtension, content: string}> { + const extension = path.extname(entrypoint); + if (extension !== '.js' && extension !== '.ts' && extension !== '.mjs') { + throw new Error(`Unsupported file extension for ${entrypoint}`); + } + try { + const content = await fsAsync.readFile(entrypoint, { encoding: 'utf-8' }); + return { extension: extension as SupportedFileExtension, content }; + } catch (err) { + throw new DependencyParseError(entrypoint, [entrypoint], [], []); + } + } - private async validateFileAsync (filePath: string): Promise<{ filePath: string, content: string }> { - const extension = path.extname(filePath) - if (extension !== '.js' && extension !== '.ts' && extension !== '.mjs') { - throw new Error(`Unsupported file extension for ${filePath}`) - } - try { - const content = await fsAsync.readFile(filePath, { encoding: 'utf-8' }) - return { filePath, content } - } catch (err) { - throw new DependencyParseError(filePath, [filePath], [], []) - } - }
158-158: Consider handling cache misses more explicitlyThe conditional logic
cache?.resolvedDependencies ?? this.resolver.resolveDependenciesForFilePath(...)could be clearer to understand the cache usage.- const resolvedDependencies = cache?.resolvedDependencies ?? - this.resolver.resolveDependenciesForFilePath(item.filePath, module.dependencies) + const resolvedDependencies = cache && cache.resolvedDependencies + ? cache.resolvedDependencies + : this.resolver.resolveDependenciesForFilePath(item.filePath, module.dependencies)
159-174: Add consistent path normalizationYou're using
pathToPosixin some places (lines 161, 173) but not others. Consider normalizing paths consistently throughout the method to avoid potential path-related issues on different platforms.for (const dep of resolvedDependencies.missing) { missingFiles.add(pathToPosix(dep.filePath)) } this.cache.set(item.filePath, { module, resolvedDependencies }) for (const dep of resolvedDependencies.local) { - if (resultFileSet.has(dep.sourceFile.meta.filePath)) { + const normalizedFilePath = pathToPosix(dep.sourceFile.meta.filePath) + if (resultFileSet.has(normalizedFilePath)) { continue } - const filePath = dep.sourceFile.meta.filePath - files.add(filePath) + files.add(normalizedFilePath) } resultFileSet.add(pathToPosix(item.filePath))
146-151: Consider adding logging for debuggingThis process of parsing and resolving dependencies can be complex. Adding optional debug logging would help with troubleshooting issues during development or customer support.
+ // Add a debug logger property to the class + private debug(message: string): void { + if (this.options?.debug) { + console.debug(`[Parser] ${message}`); + } + } // Inside getFilesAndDependencies method const item = await this.validateFileAsync(file) + this.debug(`Validating file: ${file}`); const cache = this.cache.get(item.filePath) + this.debug(`Cache ${cache ? 'hit' : 'miss'} for: ${item.filePath}`); const { module, error } = cache !== undefined ? cache : Parser.parseDependencies(item.filePath, item.content)
175-178: Missing file reporting improvementWhen throwing an error for missing files, you're using
paths.join(', ')as the primary file path in the error. This could be confusing because it's a comma-separated list of all input paths, not the specific file that had missing dependencies.if (missingFiles.size) { - throw new DependencyParseError(paths.join(', '), Array.from(missingFiles), [], []) + throw new DependencyParseError('Multiple files', Array.from(missingFiles), [], []) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts(1 hunks)packages/cli/src/services/check-parser/parser.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/services/check-parser/tests/parse-files.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test - windows-latest
- GitHub Check: test - ubuntu-latest
🔇 Additional comments (2)
packages/cli/src/services/check-parser/parser.ts (2)
3-3: Good use of modern fs/promises APIGreat choice to utilize the promise-based filesystem API for asynchronous operations in the new methods, which is more modern than callback-based approaches.
181-201: Consider optimization for glob pattern searchingAs suggested in a previous review comment, it might be more efficient to invoke glob once with an array of patterns instead of individually.
Also, the current error handling is silent for non-glob paths - it would be helpful to log or provide better feedback when a specified path doesn't exist.
private async getFilesFromPaths (paths: string[]): Promise<string[]> { + const directoryPatterns: [string, string][] = []; + const filePaths: string[] = []; + const globPatterns: [string, string][] = []; + const files = paths.map(async (currPath) => { const normalizedPath = pathToPosix(currPath) try { const stats = await fsAsync.lstat(normalizedPath) if (stats.isDirectory()) { - return findFilesWithPattern(normalizedPath, '**/*.{js,ts,mjs}', []) + directoryPatterns.push([normalizedPath, '**/*.{js,ts,mjs}']); + return []; } - return [normalizedPath] + filePaths.push(normalizedPath); + return []; } catch (err) { if (normalizedPath.includes('*') || normalizedPath.includes('?') || normalizedPath.includes('{')) { - return findFilesWithPattern(process.cwd(), normalizedPath, []) + globPatterns.push([process.cwd(), normalizedPath]); + return []; } else { + console.warn(`Path not found: ${normalizedPath}`); return [] } } }) await Promise.all(files) + // Process all directory patterns at once + const directoryFiles = directoryPatterns.length > 0 + ? await Promise.all(directoryPatterns.map(([dir, pattern]) => findFilesWithPattern(dir, pattern, []))) + : []; + + // Process all glob patterns at once + const globFiles = globPatterns.length > 0 + ? await Promise.all(globPatterns.map(([dir, pattern]) => findFilesWithPattern(dir, pattern, []))) + : []; + + return [...filePaths, ...directoryFiles.flat(), ...globFiles.flat()]; - const filesArray = await Promise.all(files) - return filesArray.flat() }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/cli/src/services/check-parser/parser.ts (4)
131-179: Solid implementation of multi-path dependency resolutionThe
getFilesAndDependenciesmethod effectively manages the complexity of resolving multiple file paths and their dependencies. It properly handles caching, error collection, and dependency traversal.One performance consideration:
Consider processing files in parallel batches rather than serially to improve performance for large sets of files:
- for (const file of files) { + // Process files in parallel batches to improve performance + const batchSize = 10; // Adjust based on expected file count + for (let i = 0; i < files.size; i += batchSize) { + const batch = Array.from(files).slice(i, i + batchSize); + const results = await Promise.all(batch.map(async (file) => { if (resultFileSet.has(file)) { - continue + return null; } if (file.endsWith('.json')) { // Holds info about the main file and doesn't need to be parsed resultFileSet.add(file) - continue + return null; } - const item = await this.validateFileAsync(file) + try { + const item = await this.validateFileAsync(file)
175-177: Consider more detailed error message for missing filesThe error message could be more informative by including the count of missing files.
if (missingFiles.size) { - throw new DependencyParseError(paths.join(', '), Array.from(missingFiles), [], []) + throw new DependencyParseError( + paths.join(', '), + Array.from(missingFiles), + [], + [], + `Found ${missingFiles.size} missing file${missingFiles.size > 1 ? 's' : ''}` + ) }
191-195: Good handling of glob patternsThe implementation correctly addresses the previous review comment by checking for glob characters before treating a path as a glob pattern. Using
process.cwd()as the base directory for glob patterns is more intuitive than using__dirname.Consider adding a warning message to inform users when a path doesn't exist and isn't a glob pattern:
if (normalizedPath.includes('*') || normalizedPath.includes('?') || normalizedPath.includes('{')) { return findFilesWithPattern(process.cwd(), normalizedPath, []) } else { + console.warn(`Path not found: ${normalizedPath}`) return [] }
181-201:❓ Verification inconclusive
Good implementation of file path resolution with glob support
The
getFilesFromPathsmethod effectively handles directories, files, and glob patterns. The implementation correctly addresses previous review comments by checking for glob characters and using the current working directory as the base for glob patterns.One optimization opportunity:
You could combine similar glob patterns to reduce the number of file system operations, as suggested in a previous review comment:
- const files = paths.map(async (currPath) => { + // Group paths by type to reduce file system operations + const directories: string[] = []; + const globPatterns: string[] = []; + const individualFiles: string[] = []; + + // First categorize paths + for (const currPath of paths) { const normalizedPath = pathToPosix(currPath) try { const stats = await fsAsync.lstat(normalizedPath) if (stats.isDirectory()) { - return findFilesWithPattern(normalizedPath, '**/*.{js,ts,mjs}', []) + directories.push(normalizedPath); + } else { + individualFiles.push(normalizedPath); } - return [normalizedPath] } catch (err) { if (normalizedPath.includes('*') || normalizedPath.includes('?') || normalizedPath.includes('{')) { - return findFilesWithPattern(process.cwd(), normalizedPath, []) + globPatterns.push(normalizedPath); } else { - return [] + // Path doesn't exist and isn't a glob pattern } } + } + + // Then process each category efficiently + const results = await Promise.all([ + // Process all directories with a single glob call if possible + directories.length > 0 + ? findFilesWithPattern(directories, '**/*.{js,ts,mjs}', []) + : [], + // Process all glob patterns with a single call + globPatterns.length > 0 + ? findFilesWithPattern(process.cwd(), globPatterns, []) + : [], + // Individual files don't need further processing + individualFiles + ]); + + return results.flat();Note: This optimization assumes that
findFilesWithPatterncan accept an array of base directories or patterns, which may require modifications to that function.
🏁 Script executed:
#!/bin/bash # Check the implementation of findFilesWithPattern to see if it supports arrays grep -r "function findFilesWithPattern" --include="*.ts" --include="*.js" .Length of output: 158
Attention: Potential Optimization via Grouping Paths
The current implementation in
packages/cli/src/services/check-parser/parser.ts(lines 181–201) effectively handles directories, files, and glob patterns. Great work on addressing past concerns regarding glob resolution by falling back on the current working directory when necessary.One potential improvement is to reduce the number of file system operations by grouping paths into categories (directories, glob patterns, and individual files) and processing them in batches. However, this approach assumes that the underlying
findFilesWithPatternfunction (defined inpackages/cli/src/services/util.ts) can handle arrays as input for its base directory or pattern parameter. From the available information, it appears thatfindFilesWithPatterncurrently expects a single string argument rather than an array.
- Action Required: If you wish to adopt the grouping optimization, you'll first need to modify
findFilesWithPatternto support array inputs. Otherwise, the current approach remains robust.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts(1 hunks)packages/cli/src/services/check-parser/parser.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/services/check-parser/tests/parse-files.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test - windows-latest
🔇 Additional comments (3)
packages/cli/src/services/check-parser/parser.ts (3)
3-3: Good addition of async file system operationsAdding the
fs/promisesimport enables modern async/await file operations, which is an improvement over the synchronous operations used elsewhere in the file.
11-11: Good refactoring of utility functionsImporting
findFilesWithPatternandpathToPosixfrom the util module is a good practice for code reuse and maintainability.
118-129: Well-structured async file validation methodThe async version of file validation properly handles error cases and maintains consistency with the existing synchronous implementation. The error handling appropriately throws a
DependencyParseErrorwith the relevant file information.
Parse now has a new method that allows the user to get all files and dependencies by providing a list of directories, files or globs
Affected Components
Notes for the Reviewer
This is the first PR for multiple file/type input for code bundling.
Following PRs
This method is not currently being used in any place of the code, it's sitting idle until we implement PWT native