-
-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add preserveConfigExtension
option
#232
base: main
Are you sure you want to change the base?
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the semicolon makes it inconsistent with the rest of the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
preserveConfigExtension
parameter to theOptions
interface 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?: boolean
src/output.ts (1)
33-42
: Consider updating tests and documentation.The changes to the
outputEntry
function look good. However, to ensure comprehensive coverage:
- Update any existing unit tests for this function to include cases with
preserveConfigExtension
set to bothtrue
andfalse
.- 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 optionpreserveConfigExtension
added.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
preserveConfigExtension
parameter to theOptions
interface 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
preserveConfigExtension
parameter with a default value offalse
is 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
configModPath
based on thepreserveConfigExtension
parameter. 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
preserveConfigExtension
option as requested in issue #231. The implementation is backward-compatible and localized to theoutputEntry
function. 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 ofpreserveConfigExtension
option looks good.The changes in this file successfully implement the new
preserveConfigExtension
option 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 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.ts
src/output.ts
docs/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
preserveConfigExtension
option to the build process, which allows users to preserve the file extension of thevelite.config
file 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:
preserveConfigExtension
option to the build process, allowing the preservation of thevelite.config
file extension in the outputindex.d.ts
file.Documentation:
preserveConfigExtension
option, explaining its purpose and default value.Summary by CodeRabbit
New Features
preserveConfigExtension
, in the API reference for thebuild
function, allowing users to retain the extension of thevelite.config
file in the output.Documentation
preserveConfigExtension
parameter and its default value.