feat: add preserveConfigExtension option
#232
Conversation
Reviewer's Guide by SourceryThis pull request adds a new Class diagram for updated Options interfaceclassDiagram
class Options {
boolean strict
boolean preserveConfigExtension
}
class build {
+build(options: Options): Promise<Record<string, unknown>>
}
Options <|-- build: uses
class outputEntry {
+outputEntry(dest: string, configPath: string, collections: Collections, preserveConfigExtension: boolean): Promise<void>
}
build --> outputEntry: calls
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe pull request introduces a new optional parameter, Changes
Assessment against linked issues
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 (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @huntabyte - 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: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| * Useful if `"moduleResolution": "NodeNext"` where extension-less files aren't supported. | ||
| * @default false | ||
| */ | ||
| preserveConfigExtension?: boolean |
There was a problem hiding this comment.
suggestion (documentation): Consider adding an explicit type annotation for consistency
Other options in this interface, like logLevel, have explicit type annotations. Adding : boolean here would maintain consistency.
| preserveConfigExtension?: boolean | |
| preserveConfigExtension?: boolean; |
There was a problem hiding this comment.
Adding the semicolon makes it inconsistent with the rest of the interface.
There was a problem hiding this comment.
Thank you for your feedback, we will generate fewer comments like this in the future according to the following instructions:
- Ensure the comment addresses a significant issue or improvement in the code or documentation.
- Avoid comments that suggest minor stylistic changes unless they significantly impact readability or maintainability.
- Check if the comment provides a clear rationale for the suggestion and if it aligns with existing code conventions or guidelines.
![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
🧹 Outside diff range and nitpick comments (3)
docs/reference/api.md (1)
104-109: LGTM! Documentation for the new parameter is clear and addresses the PR objectives.The addition of the
preserveConfigExtensionparameter to theOptionsinterface is well-documented and aligns perfectly with the PR objectives. It provides a clear solution for users who need to supportmoduleResolution: "NodeNext".Consider adding a cross-reference to the related configuration in the TypeScript documentation for
moduleResolution. This could provide users with more context on when and why they might need to use this option. For example:/** * If true, preserves the extension of the `velite.config.[ts|js]` file in the output `index.d.ts` file. * Useful if `"moduleResolution": "NodeNext"` where extension-less files aren't supported. + * For more information, see: https://www.typescriptlang.org/tsconfig#moduleResolution * @default false */ preserveConfigExtension?: booleansrc/output.ts (1)
33-42: Consider updating tests and documentation.The changes to the
outputEntryfunction look good. However, to ensure comprehensive coverage:
- Update any existing unit tests for this function to include cases with
preserveConfigExtensionset to bothtrueandfalse.- If there's separate documentation for the API, make sure to update it to reflect this new parameter and its behavior.
src/build.ts (1)
244-249: LGTM: New optionpreserveConfigExtensionadded.The new option is well-documented and addresses the issue mentioned in the PR objectives. It provides a solution for users using
"moduleResolution": "NodeNext".Consider adding a link to the relevant documentation or issue in the comment for future reference:
/** * If true, preserves the extension of the `velite.config` file in the output `index.d.ts` file. * Useful if `"moduleResolution": "NodeNext"` where extension-less files aren't supported. + * @see https://github.com/zce/velite/issues/231 * @default false */ preserveConfigExtension?: boolean
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- docs/reference/api.md (1 hunks)
- src/build.ts (3 hunks)
- src/output.ts (1 hunks)
🔇 Additional comments (6)
docs/reference/api.md (1)
Line range hint
1-109: Overall, the documentation update is well-implemented and focused.The addition of the
preserveConfigExtensionparameter to theOptionsinterface is the only change in this document, which aligns well with the PR objectives. The new parameter is clearly documented, and its purpose is well-explained. The rest of the document remains unchanged, maintaining consistency and focus on the specific feature addition.src/output.ts (3)
31-33: LGTM! New parameter added to supportmoduleResolution: 'NodeNext'.The addition of the
preserveConfigExtensionparameter with a default value offalseis a good approach. It addresses the issue described in #231 while maintaining backward compatibility. The JSDoc comment clearly explains the purpose of the new parameter.
40-42: Conditional logic correctly implements the new feature.The new conditional logic effectively handles the
configModPathbased on thepreserveConfigExtensionparameter. Whentrue, it preserves the file extension, and whenfalse, it removes it as before. This implementation aligns well with the PR objectives.
31-42: Summary: New option successfully implements requested feature.The changes in this file successfully implement the
preserveConfigExtensionoption as requested in issue #231. The implementation is backward-compatible and localized to theoutputEntryfunction. This new feature will allow users to preserve the file extension when usingmoduleResolution: 'NodeNext', addressing the reported issue without affecting existing functionality.Great job on this implementation!
src/build.ts (2)
3-4: LGTM: Import statement rearrangement.The rearrangement of import statements is a minor change that doesn't affect functionality. It likely aligns with code style or linting rules.
Line range hint
1-291: Summary: Implementation ofpreserveConfigExtensionoption looks good.The changes in this file successfully implement the new
preserveConfigExtensionoption as described in the PR objectives. The implementation is minimal, focused, and maintains backward compatibility.Next steps:
- Review the results of the verification script to ensure complete implementation.
- Update or add tests for the new option.
- Update the documentation to include information about this new option.
- Consider adding a changelog entry for this feature.
| } | ||
|
|
||
| /** | ||
| * Build contents | ||
| * @param options build options | ||
| */ | ||
| export const build = async (options: Options = {}): Promise<Record<string, unknown>> => { | ||
| const { config: configFile, clean, logLevel, strict } = options | ||
| const { config: configFile, clean, logLevel, strict, preserveConfigExtension = false } = options |
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Add Tests for preserveConfigExtension Option
The preserveConfigExtension option is implemented in the build and output functions and is documented in the API reference. However, no tests have been found to verify its functionality.
- Implementation Locations:
src/build.tssrc/output.tsdocs/reference/api.md
Recommendation:
Please add tests to ensure that the preserveConfigExtension option behaves as expected and maintains the desired functionality.
🔗 Analysis chain
LGTM: preserveConfigExtension option implemented in build function.
The changes correctly implement the new option in the build function and pass it to the outputEntry function. The default value of false ensures backward compatibility.
To ensure the preserveConfigExtension option is correctly implemented throughout the codebase, let's verify its usage:
Please review the results of these checks to ensure the option is fully implemented and tested.
Also applies to: 281-281
Action Required: Update and Re-run Verification Scripts
The previous verification scripts failed because ripgrep does not recognize typescript as a valid file type. Please use ts instead. Below are the updated scripts:
Please execute these updated scripts to continue the verification process.
Action Required: Further Verification Needed for preserveConfigExtension Implementation
The recent verification did not locate any instances of preserveConfigExtension within the src or test directories. To thoroughly confirm its implementation, please execute the following comprehensive search scripts:
Please run these scripts to gather additional information on the usage and implementation of the preserveConfigExtension option.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of preserveConfigExtension option
# Test 1: Check if outputEntry function signature has been updated
echo "Test 1: Checking outputEntry function signature"
rg --type typescript "function outputEntry.*preserveConfigExtension" src
# Test 2: Verify preserveConfigExtension is used in the output generation logic
echo "Test 2: Checking usage of preserveConfigExtension in output generation"
rg --type typescript "preserveConfigExtension.*\.js[\"']?" src
# Test 3: Ensure tests have been updated or added for the new option
echo "Test 3: Checking for updated or new tests"
rg --type typescript "test.*preserveConfigExtension" test
Length of output: 655
Script:
#!/bin/bash
# Description: Verify the implementation of preserveConfigExtension option with corrected file types
# Test 1: Check if outputEntry function signature has been updated
echo "Test 1: Checking outputEntry function signature"
rg --type ts "function outputEntry.*preserveConfigExtension" src
# Test 2: Verify preserveConfigExtension is used in the output generation logic
echo "Test 2: Checking usage of preserveConfigExtension in output generation"
rg --type ts "preserveConfigExtension.*\.js['\"]?" src
# Test 3: Ensure tests have been updated or added for the new option
echo "Test 3: Checking for updated or new tests"
rg --type ts "test.*preserveConfigExtension" test
Length of output: 526
Script:
#!/bin/bash
# Description: Comprehensive search for 'preserveConfigExtension' usage across the entire codebase
echo "Test 1: Searching for 'preserveConfigExtension' in all files"
rg "preserveConfigExtension" .
echo "Test 2: Searching for 'preserveConfigExtension' in non-TypeScript files"
rg "preserveConfigExtension" --type-not ts
echo "Test 3: Searching for any references or related configurations"
rg "preserveConfigExtension" src || rg "preserveConfigExtension" test
Length of output: 1901
Closes #231
Summary by Sourcery
Add a new
preserveConfigExtensionoption to the build process, which allows users to preserve the file extension of thevelite.configfile in the output. This is particularly useful for environments using"moduleResolution": "NodeNext"where extension-less files are not supported. Update the API documentation to reflect this new option.New Features:
preserveConfigExtensionoption to the build process, allowing the preservation of thevelite.configfile extension in the outputindex.d.tsfile.Documentation:
preserveConfigExtensionoption, explaining its purpose and default value.Summary by CodeRabbit
New Features
preserveConfigExtension, in the API reference for thebuildfunction, allowing users to retain the extension of thevelite.configfile in the output.Documentation
preserveConfigExtensionparameter and its default value.