breaking: upgrade chokidar v4 #237
Conversation
Reviewer's Guide by SourceryThis pull request upgrades the chokidar dependency to version 4, replaces micromatch with picomatch, and updates the file watching logic in the build process. It also includes minor changes to timestamp handling and adds a new utility function for pattern matching. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe pull request introduces several modifications focused on enhancing the handling of timestamps in the project. Key changes include updates to the Changes
Possibly related PRs
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 (
|
There was a problem hiding this comment.
Hey @zce - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| import { relative } from 'node:path' | ||
| import pm from 'picomatch' | ||
|
|
||
| export const matchPatterns = (input: string, patterns: string | string[], base?: string) => { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the matchPatterns function into smaller, focused functions to improve code organization and maintainability.
The matchPatterns function introduces unnecessary complexity by handling multiple concerns in a single function. Here's a suggestion to simplify and improve the code:
- Split the function into smaller, focused functions:
import { relative } from 'node:path'
import pm from 'picomatch'
const normalizeInput = (input: string, base?: string): string => {
if (base != null) {
input = relative(base, input).replace(/^\.[\\/]/, '')
}
return input.replaceAll('\\', '/')
}
const separatePatterns = (patterns: string | string[]): { normal: string[], negated: string[] } => {
const list = Array.isArray(patterns) ? patterns : [patterns]
return list.reduce((acc, p) => {
acc[p.startsWith('!') ? 'negated' : 'normal'].push(p)
return acc
}, { normal: [] as string[], negated: [] as string[] })
}
export const matchPatterns = (input: string, patterns: string | string[], base?: string): boolean => {
const normalizedInput = normalizeInput(input, base)
const { normal, negated } = separatePatterns(patterns)
return normal.some(i => pm(i)(normalizedInput)) && negated.every(i => pm(i)(normalizedInput))
}- Consider using simpler array methods instead of
reducefor pattern separation:
const separatePatterns = (patterns: string | string[]): { normal: string[], negated: string[] } => {
const list = Array.isArray(patterns) ? patterns : [patterns]
return {
normal: list.filter(p => !p.startsWith('!')),
negated: list.filter(p => p.startsWith('!')).map(p => p.slice(1))
}
}- For future extensions, consider creating separate utility functions instead of adding more complexity to
matchPatterns. This will help maintain the single responsibility principle and prevent this file from becoming a catch-all for string matching utilities.
These changes improve readability, maintainability, and adhere better to the single responsibility principle while preserving the original functionality.
| awaitWriteFinish: { stabilityThreshold: 50, pollInterval: 10 } | ||
| }).on('all', async (event, filename) => { | ||
| if (event === 'addDir' || event === 'unlinkDir') return // ignore dir changes | ||
| if (event === 'addDir' || event === 'unlinkDir') return |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (event === 'addDir' || event === 'unlinkDir') return | |
| if (event === 'addDir' || event === 'unlinkDir') { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| logger.info('velite config changed, restarting...') | ||
| watcher.close() | ||
| return build({ config: config.configPath, clean: false, watch: true }) | ||
| } | ||
|
|
||
| // skip if filename not match any collection pattern | ||
| if (!matchPatterns(filename, patterns)) return |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!matchPatterns(filename, patterns)) return | |
| if (!matchPatterns(filename, patterns)) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
|
|
||
| // remove changed file cache | ||
| for (const [key, value] of config.cache.entries()) { | ||
| if (value === fullpath) config.cache.delete(key) |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (value === fullpath) config.cache.delete(key) | |
| if (value === fullpath) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
src/utils.ts (3)
5-7: LGTM: Good handling of input patterns. Consider creating an issue for the TODO.The array initialization ensures uniform handling of both string and string[] inputs, which is a good practice. The TODO comment about using
groupByin the future is a good reminder for potential optimization.Would you like me to create a GitHub issue to track the TODO for implementing
groupByin the future?
16-20: LGTM: Path handling looks good. Consider using optional chaining.The path handling logic is correct and necessary for consistent matching across different systems. A minor suggestion would be to use optional chaining for a more concise check.
Consider updating the condition:
if (base != null) { input = relative(base, input).replace(/^\.[\\/]/, '') }to:
input = base ? relative(base, input).replace(/^\.[\\/]/, '') : inputThis change eliminates the need for a separate
ifblock, making the code slightly more concise.
22-23: LGTM: Efficient pattern matching logic. Consider adding a comment for clarity.The pattern matching logic using
picomatchis correct and efficient. The use ofsomeandeveryis a good way to implement the matching logic.For improved readability, especially for developers who might not be familiar with
picomatch, consider adding a brief comment explaining the logic:// Match if the input matches any normal pattern and doesn't match any negated pattern return normal.some(i => pm(i)(input)) && negated.every(i => !pm(i)(input));Also, note that the negated patterns should use
!pm(i)(input)instead ofpm(i)(input)to correctly implement the negation.test/utils.ts (2)
6-6: Consider simplifying the describe function.The
describefunction is currently defined as an async function with an unused parametert. Since there are no async operations at this level, you can simplify it.Consider changing the line to:
describe('matchPatterns function', () => {
7-20: Consider enhancing test case naming and coverage.While the current test cases are good, consider the following improvements:
Use more descriptive names for test cases. For example:
it('matches a single pattern correctly', () => {...}) it('handles multiple patterns as expected', () => {...}) it('correctly applies negated patterns', () => {...})Add more edge cases to improve coverage:
- Test with an empty pattern array
- Test with conflicting patterns (e.g.,
['**/*.js', '!**/*.js'])- Test with a file path containing special characters
Consider using
test.eachfor parameterized tests to reduce duplication.Would you like me to provide examples for these additional test cases?
docs/guide/last-modified.md (1)
60-60: Approve change with suggestion for error loggingThe modification to use
Date.now()as a fallback is a good defensive programming practice. It ensures that a valid timestamp is always returned, even if the Git command fails to provide one.However, to aid in debugging, consider adding some error logging when
stdoutis empty or falsy. This will help identify issues with Git command execution or file paths while still maintaining the robust behavior.Consider modifying the code as follows:
- return new Date(stdout || Date.now()).toISOString() + if (!stdout) { + console.warn(`Git timestamp not available for ${meta.path}. Using current time.`); + } + return new Date(stdout || Date.now()).toISOString()This change will log a warning when falling back to the current time, which can be helpful for identifying and addressing any underlying issues.
package.json (1)
52-62: Summary of dependency changesThe changes in this PR include:
- Replacing micromatch with picomatch (including type definitions)
- Upgrading chokidar to version 4
These updates align with the PR objectives and may improve the project's functionality. However, they also introduce potential breaking changes that need to be carefully managed.
To ensure a smooth transition:
- Review the changelogs for both picomatch and chokidar v4 for any breaking changes.
- Update all instances of micromatch usage to picomatch throughout the codebase.
- Test thoroughly, especially file watching functionality that relies on chokidar.
- Consider updating the project's documentation to reflect these dependency changes.
examples/nextjs/velite.config.ts (1)
33-33: Approve with suggestions: Improved robustness, but consider error handlingThe change improves the robustness of the
timestampfunction by adding a fallback toDate.now()when the Git command fails or returns an empty string. This ensures that a valid date is always returned.However, silently falling back to the current date might mask underlying issues with Git or file tracking. Consider the following improvements:
- Add a warning log when falling back to the current date:
const gitDate = stdout.trim(); if (!gitDate) { console.warn(`Unable to get Git timestamp for ${meta.path}. Falling back to current date.`); } return new Date(gitDate || Date.now()).toISOString();
- In development environments, consider throwing an error instead of silently falling back:
const gitDate = stdout.trim(); if (!gitDate) { if (process.env.NODE_ENV === 'development') { throw new Error(`Unable to get Git timestamp for ${meta.path}`); } console.warn(`Unable to get Git timestamp for ${meta.path}. Falling back to current date.`); } return new Date(gitDate || Date.now()).toISOString();These suggestions will help with debugging and make it more apparent when the fallback is being used.
examples/basic/velite.config.js (1)
33-33: Approve change with suggestions for improvementThe modification to use
Date.now()as a fallback is a good improvement in error handling, ensuring that a valid date is always returned. However, consider the following suggestions:
Use file system metadata (e.g., last modified time) as a more accurate fallback instead of the current time. This would better represent the actual last update time of the file.
Add a warning log when falling back to indicate that the Git command failed to produce a result.
Here's a suggested implementation incorporating these improvements:
const fs = require('fs').promises; // ... (rest of the code) const timestamp = () => s .custom(i => i === undefined || typeof i === 'string') .transform(async (value, { meta, addIssue }) => { if (value != null) { addIssue({ fatal: false, code: 'custom', message: '`s.timestamp()` schema will resolve the value from `git log -1 --format=%cd`' }) } try { const { stdout } = await execAsync(`git log -1 --format=%cd ${meta.path}`) return new Date(stdout.trim()).toISOString() } catch (error) { console.warn(`Failed to get Git timestamp for ${meta.path}. Falling back to file's last modified time.`) const stats = await fs.stat(meta.path) return new Date(stats.mtime).toISOString() } })This implementation attempts to use the Git timestamp first, falls back to the file's last modified time if that fails, and logs a warning in the fallback case.
📜 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)
- docs/guide/last-modified.md (1 hunks)
- docs/other/snippets.md (1 hunks)
- examples/basic/velite.config.js (1 hunks)
- examples/nextjs/velite.config.ts (1 hunks)
- package.json (1 hunks)
- src/build.ts (3 hunks)
- src/utils.ts (1 hunks)
- test/utils.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (15)
src/utils.ts (2)
1-4: LGTM: Imports and function signature are well-defined.The imports are appropriate for the function's purpose, and the function signature is clear and well-typed. The use of an optional parameter for
baseis a good practice, allowing flexibility in function usage.
1-23: Overall, the implementation ofmatchPatternsis solid and efficient.The function effectively handles pattern matching with support for negated patterns and relative paths. It makes good use of TypeScript features and efficient array methods. The suggestions provided in the review are minor and aimed at improving readability and conciseness.
Great job on implementing this utility function!
test/utils.ts (3)
1-4: LGTM: Imports are correct and follow good practices.The imports are well-structured, using Node.js built-in modules for testing and assertions. The relative import for
matchPatternslooks correct.
7-20: LGTM: Well-structured test cases with good coverage.The test cases cover important scenarios for the
matchPatternsfunction, including single patterns, multiple patterns, and negated patterns. The assertions are correct and test both positive and negative cases.
1-21: Overall, well-structured and effective test suite with room for minor enhancements.This new test file for the
matchPatternsfunction is well-organized and covers key scenarios. The use of Node.js built-in testing modules is commendable. To further improve:
- Simplify the
describefunction by removing the async keyword and unused parameter.- Enhance test case naming for better clarity.
- Consider adding more edge cases to increase coverage.
These changes will make the test suite more robust and easier to maintain in the future.
package.json (3)
62-62: LGTM: Addition of picomatchThe addition of picomatch as a replacement for micromatch is a good update. This change aligns with the removal of micromatch mentioned in the AI summary.
Please ensure that all instances of micromatch usage in the codebase have been updated to use picomatch. You can use the following script to check for any remaining micromatch usage and find places where picomatch should be used:
✅ Verification successful
Verified: micromatch Successfully Replaced with picomatch
All instances of micromatch have been removed from the codebase, and picomatch has been correctly integrated as its replacement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining micromatch usage and find potential picomatch usage # Search for micromatch imports or usage echo "Checking for remaining micromatch usage:" rg --type typescript --type javascript 'micromatch' # Search for potential places to use picomatch echo "\nPotential places to use picomatch:" rg --type typescript --type javascript '(glob|pattern|wildcard|match)'Length of output: 368
Script:
#!/bin/bash # Description: Check for any remaining micromatch usage and find potential places to use picomatch # Search for micromatch imports or usage in JavaScript and TypeScript files echo "Checking for remaining micromatch usage:" rg 'micromatch' --glob '*.js' --glob '*.ts' # Search for potential places to use picomatch in JavaScript and TypeScript files echo -e "\nPotential places to use picomatch:" rg '(glob|pattern|wildcard|match)' --glob '*.js' --glob '*.ts'Length of output: 4807
52-52: LGTM: Addition of @types/picomatchThe addition of
@types/picomatchaligns with the transition from micromatch to picomatch. This is a good update to maintain type safety with the new dependency.Please verify that all instances of micromatch usage in the codebase have been updated to use picomatch. You can use the following script to check for any remaining micromatch usage:
54-54: LGTM: Upgrade of chokidar to v4The upgrade of chokidar to version 4.0.1 aligns with the PR objective. This is a major version upgrade, which may introduce breaking changes.
Please review the chokidar changelog for any breaking changes and ensure that the codebase is compatible with the new version. You can use the following script to find chokidar usage in the codebase:
docs/other/snippets.md (1)
51-51: Improved robustness in timestamp handlingThis change enhances the reliability of the
timestampschema by providing a fallback to the current timestamp whenstdoutis empty or falsy. This ensures that a valid date is always returned, preventing potential issues with invalid dates in the system.src/build.ts (6)
12-12: LGTM: Updated import statementThe import statement has been correctly updated to import
matchPatternsfrom./utils.
179-179: LGTM: Collecting patterns from collectionsThe patterns are correctly collected from the collections for use in the watcher.
192-196: LGTM: Handling configuration changesThe code correctly handles configuration changes by restarting the build process when a config file changes.
198-200: Ensure consistent usage ofmatchPatternsfunctionThis usage of
matchPatternsis similar to the one noted earlier. Please refer to the previous comment to ensure consistency.
201-205: LGTM: Clearing cache entries for the changed fileThe cache is properly updated by removing entries related to the changed file, ensuring that stale data is not used.
207-209: LGTM: Rebuilding upon file changesThe code successfully triggers a rebuild when a change is detected in watched files, maintaining up-to-date output.
| const { normal, negated } = list.reduce( | ||
| (acc, p) => { | ||
| acc[p.startsWith('!') ? 'negated' : 'normal'].push(p) | ||
| return acc | ||
| }, | ||
| { normal: [] as string[], negated: [] as string[] } | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring the reducer for improved readability.
While the current implementation using reduce is efficient for categorizing patterns, it could be made more readable by extracting the reducer logic into a separate function.
Here's a suggested refactor:
const categorizePatterns = (patterns: string[]) => {
const categorizePattern = (acc: { normal: string[], negated: string[] }, pattern: string) => {
const category = pattern.startsWith('!') ? 'negated' : 'normal';
acc[category].push(pattern);
return acc;
};
return patterns.reduce(categorizePattern, { normal: [], negated: [] });
};
const { normal, negated } = categorizePatterns(list);This refactoring improves readability by giving clear names to the operations being performed and separating concerns.
| const watcher = watch(['.', ...configImports], { | ||
| cwd: root, | ||
| ignored: /(^|[\/\\])[\._]./, // ignore dot & underscore files | ||
| ignoreInitial: true, // ignore initial scan | ||
| ignoreInitial: true, | ||
| awaitWriteFinish: { stabilityThreshold: 50, pollInterval: 10 } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refining the watch paths to improve performance
Watching the entire root directory '.' may be unnecessary and could impact performance. Consider specifying only the necessary directories or patterns to watch, to avoid excessive file system events.
| if (changed != null && !matchPatterns(changed, pattern, root) && resolved.has(name)) { | ||
| // skip collection if changed file not match | ||
| logger.log(`skipped resolve '${name}', using previous resolved`) |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent usage of matchPatterns function detected
The matchPatterns function is called with different numbers of arguments across the codebase:
-
In
src/build.ts:matchPatterns(changed, pattern, root)matchPatterns(filename, patterns)
-
In
test/utils.ts:- Various calls with two arguments, such as
matchPatterns('foo/bar.js', '**/*.js')
- Various calls with two arguments, such as
Please ensure that the function is used consistently with the correct number and order of arguments.
🔗 Analysis chain
Verify consistent usage of matchPatterns function
The matchPatterns function is called with different numbers of arguments in lines 101 and 199. Please ensure that the function supports these usages and that the arguments are in the correct order.
Run the following script to verify the usage of matchPatterns across the codebase:
Re-run verification for consistent usage of matchPatterns
Please execute the following script to gather the necessary information:
Re-run verification for consistent usage of matchPatterns
Please execute the following corrected script to gather the necessary information:
Re-run verification for consistent usage of matchPatterns using rg
Please execute the following script to gather the necessary information:
Re-run verification for consistent usage of matchPatterns using rg with file extensions
Please execute the following corrected script to gather the necessary information:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to 'matchPatterns' and display their arguments.
# Test: Ensure 'matchPatterns' is consistently used with the correct number of arguments.
ast-grep --lang typescript --pattern 'matchPatterns($$ARGS)'
Length of output: 62
Script:
#!/bin/bash
# Description: Find all calls to 'matchPatterns' and display their arguments.
# Test: Ensure 'matchPatterns' is consistently used with the correct number of arguments.
ast-grep --lang typescript --pattern 'matchPatterns($$ARGS)'
Length of output: 62
Script:
#!/bin/bash
# Description: Find all calls to 'matchPatterns' and display their arguments.
# Test: Ensure 'matchPatterns' is consistently used with the correct number of arguments.
ast-grep --lang typescript --pattern 'matchPatterns($$*)'
Length of output: 59
Script:
#!/bin/bash
# Description: Find all calls to 'matchPatterns' and display their arguments using ripgrep.
# Test: Ensure 'matchPatterns' is consistently used with the correct number of arguments.
rg 'matchPatterns\(([^)]*)\)' --type typescript | grep 'matchPatterns'
Length of output: 105
Script:
#!/bin/bash
# Description: Find all calls to 'matchPatterns' and display their arguments using ripgrep.
# Test: Ensure 'matchPatterns' is consistently used with the correct number of arguments.
rg 'matchPatterns\(([^)]*)\)' -g '*.ts' -g '*.tsx'
Length of output: 773
Summary by Sourcery
Upgrade chokidar to version 4, refactor pattern matching logic using a new utility function, and update timestamp handling to default to the current date when
git logdoes not provide a date. Update dependencies and documentation to reflect these changes.New Features:
matchPatternsinsrc/utils.tsto handle pattern matching with support for negated patterns.Enhancements:
src/build.tsto use the newmatchPatternsutility for improved pattern matching.git logdoes not return a date, defaulting to the current date.Build:
chokidarto version 4.0.1, which requires Node.js version 14.16.0 or higher.micromatchwithpicomatchfor pattern matching, updating dependencies accordingly.Documentation:
docs/guide/last-modified.mdanddocs/other/snippets.mdto reflect changes in timestamp handling.Summary by CodeRabbit
New Features
Bug Fixes
Chores