Upgrade to Jiti 2 #149
Upgrade to Jiti 2 #149
Conversation
WalkthroughThe pull request introduces several changes across multiple files, focusing on enhancing the functionality and documentation of 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #149 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 22 31 +9
Branches 2 4 +2
=========================================
+ Hits 22 31 +9 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (9)
smoke-tests/smoke-test-esm-sync.mjs (2)
1-5: Consider the implications of immediate TypeScriptLoaderSync invocation.The
TypeScriptLoaderSync()function is called immediately after import. This might have unintended side effects or impact the global state. Consider moving this invocation inside the async function where it's used, to ensure better control over when it's executed.
15-20: Consider using absolute path and documenting the expected configuration.
The relative path
'./smoke-tests/config.ts'might cause issues if the working directory changes. Consider usingimport.meta.urlor__dirname(in CJS) to construct an absolute path.The expected configuration is hardcoded. While this is acceptable for a smoke test, consider adding a comment explaining why this specific configuration is expected, to aid future maintenance.
Example of using an absolute path:
import { fileURLToPath } from 'url'; import { dirname, join } from 'path'; const __filename = fileURLToPath(import.meta.url); const __dirname = dirname(__filename); const cfg = await explorer.load(join(__dirname, 'config.ts'));smoke-tests/smoke-test-cjs-sync.cjs (1)
15-20: Consider enhancing error handling for configuration loadingThe configuration loading and validation look good. However, to improve error handling and debugging, consider adding more specific error messages. For example:
const cfg = await explorer.load("./smoke-tests/config.ts"); assert.deepStrictEqual(cfg.config, { cake: "lie", }); -console.info("Loaded with CJS successfully"); +console.info("Configuration loaded and validated successfully"); +console.debug("Loaded configuration:", JSON.stringify(cfg.config, null, 2));This change would provide more detailed information about the loaded configuration, which could be helpful for debugging.
smoke-tests/smoke-test-sync.js (1)
19-25: LGTM: Comprehensive error handling with a minor suggestion.The error handling is well-implemented, logging both the error and its stack trace. Exiting the process on error is appropriate for a smoke test.
Consider adding a more specific error message to indicate that the smoke test failed:
console.error("Smoke test failed:"); console.error(error);This would make it clearer in logs that the failure is specifically from the smoke test.
package.json (1)
47-47: Approved: Node.js version requirement updatedThe minimum required Node.js version has been increased from v16 to v18. This change aligns with modern Node.js practices and allows the use of newer JavaScript features.
Consider adding a note to the changelog about this breaking change, as it may affect users on older Node.js versions.
README.md (2)
56-58: Good addition of synchronous loading information.The new section clearly explains the deprecation of the synchronous loader, which is an important change for users to be aware of. This is consistent with the PR objectives.
Consider adding a brief note recommending users to prefer the asynchronous method when possible, to further emphasize the direction of the upgrade.
60-72: Clear example of synchronous loading usage.The code example effectively demonstrates how to use the
TypeScriptLoaderSyncfor users who still need synchronous loading. This is a helpful addition to the documentation.Consider adding a comment within the code example to remind users that this method is deprecated, reinforcing the information provided in the text above. For example:
// Note: Synchronous loading is deprecated. Use asynchronous loading when possible. const cfg = explorer.load("./amazing.config.ts");lib/index.spec.ts (1)
15-15: LGTM: Test case added for TypeScriptLoaderSync.The new test case correctly verifies that
TypeScriptLoaderSyncis exported as a function, maintaining consistency with the existing test forTypeScriptLoader.Consider combining these checks into a single test case for conciseness:
it("should export the loader functions", () => { expect(typeof TypeScriptLoader).toStrictEqual("function"); expect(typeof TypeScriptLoaderSync).toStrictEqual("function"); });lib/loader.spec.ts (1)
61-69: Ensure consistent error handling across testsThere is inconsistency in how errors are handled in your tests. Some tests use
expect(...).toThrow(), while others usetry-catchblocks withfail(). Consistent error handling improves readability and maintenance.Consider standardizing the error assertions by using
expect(...).toThrow()in synchronous tests andawait expect(...).rejects.toThrow()in asynchronous tests.Also applies to: 128-136
📜 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 (10)
- .github/workflows/build.yml (1 hunks)
- README.md (3 hunks)
- lib/index.spec.ts (3 hunks)
- lib/index.ts (1 hunks)
- lib/loader.spec.ts (1 hunks)
- lib/loader.ts (1 hunks)
- package.json (1 hunks)
- smoke-tests/smoke-test-cjs-sync.cjs (1 hunks)
- smoke-tests/smoke-test-esm-sync.mjs (1 hunks)
- smoke-tests/smoke-test-sync.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (24)
lib/index.ts (1)
1-1: New synchronous loader added to exportsThe addition of
TypeScriptLoaderSyncto the exports is consistent with the PR objectives of upgrading to Jiti 2 and introducing synchronous import capabilities. This change expands the public API of the package, which is a significant modification.A few points to consider:
- Ensure that
TypeScriptLoaderSyncis properly implemented in./loader.js.- Update the package documentation (README, API docs) to reflect this new export and its usage.
- Consider adding a deprecation notice for the synchronous loader if the long-term goal is to transition fully to asynchronous imports.
- Verify that this change doesn't break backwards compatibility for existing users.
To ensure the
TypeScriptLoaderSyncis properly implemented, let's check theloader.jsfile:smoke-tests/smoke-test-esm-sync.mjs (3)
7-13: LGTM: Proper setup for cosmiconfig with TypeScript loader.The cosmiconfig setup looks correct. Using an immediately invoked async function is a good practice for handling asynchronous operations at the top level.
21-27: LGTM: Comprehensive error handling.The error handling in this segment is well-implemented:
- Success and error messages are logged appropriately.
- The error stack is logged for debugging purposes.
- The process exits with a non-zero status code on error, which is correct for a smoke test.
This approach ensures that any issues during the test will be clearly visible and will cause the process to fail, as expected in a smoke test.
1-28: Overall: Well-implemented smoke test with minor suggestions for improvement.This smoke test for ESM synchronous loading with TypeScript is well-structured and covers all necessary aspects: setup, configuration loading, assertion, and error handling. The suggestions provided earlier (regarding TypeScriptLoaderSync invocation and path resolution) are minor and aimed at improving robustness and maintainability. The test serves its purpose effectively and will help ensure the correct functioning of the ESM synchronous loading feature.
smoke-tests/smoke-test-cjs-sync.cjs (3)
7-13: LGTM: Proper setup of cosmiconfig with custom loaderThe cosmiconfig setup looks correct. You're properly using an async IIFE to handle asynchronous operations, and the custom loader for .ts files is correctly specified.
1-28: Overall assessment: Well-structured smoke test with room for minor improvementsThis smoke test for the synchronous CJS loader is well-structured and covers the basic functionality of loading and validating a TypeScript configuration file. The suggested improvements focus on enhancing error handling, improving debugging output, and verifying the necessity of the immediate TypeScriptLoaderSync invocation. These changes would make the test more robust and easier to debug without altering its core functionality.
1-5: Verify the immediate invocation of TypeScriptLoaderSyncThe imports look correct, but the immediate invocation of
TypeScriptLoaderSync()on line 5 is unusual. Typically, this function would be used as an argument to the cosmiconfig options, as seen later in the file. Consider removing this line if it's not necessary for setup.✅ Verification successful
The immediate invocation of
TypeScriptLoaderSync()is consistent with its usage in other test files and is necessary for the test setup.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if TypeScriptLoaderSync is used elsewhere in the project rg --type js --type ts "TypeScriptLoaderSync\(\)" -g '!smoke-tests/smoke-test-cjs-sync.cjs'Length of output: 333
smoke-tests/smoke-test-sync.js (3)
1-6: LGTM: Proper setup for testing both ESM and CJS modules.The code correctly sets up the environment to test both ESM and CJS module formats. The use of an async IIFE is a good practice for handling top-level await.
8-12: LGTM: Thorough assertions for loaded functions.The assertions comprehensively check both the name and type of the imported functions for ESM and CJS. This ensures that the correct loader is imported in both module formats.
18-18: LGTM: Clear success logging.The success message is concise and accurately indicates that both CJS and ESM modules were loaded successfully.
.github/workflows/build.yml (4)
45-53: Excellent addition of synchronous import tests!The three new steps significantly enhance the test coverage by including synchronous import tests for CJS, ESM, and a combined scenario. These additions align perfectly with the PR objectives of upgrading to Jiti 2 and supporting both synchronous and asynchronous imports. The workflow now provides a more comprehensive test suite, ensuring better reliability and compatibility.
Great job on maintaining consistency with the existing workflow structure and ensuring that all new steps always run.
48-50: LGTM! New step for synchronous ESM imports added.This new step complements the CJS test and aligns with the PR objective of testing synchronous imports. The use of
if: ${{ always() }}ensures comprehensive testing.Let's verify the existence of the new test file:
#!/bin/bash # Verify the existence of the new ESM sync test file if [ -f "smoke-tests/smoke-test-esm-sync.mjs" ]; then echo "ESM sync test file exists" else echo "ESM sync test file is missing" fi
51-53: LGTM! Comprehensive step for synchronous imports with both CJS and ESM added.This new step provides a thorough test covering both module systems in a single run, which is excellent for ensuring compatibility. The use of
if: ${{ always() }}ensures comprehensive testing.Let's verify the existence of the new test file:
#!/bin/bash # Verify the existence of the new combined sync test file if [ -f "smoke-tests/smoke-test-sync.js" ]; then echo "Combined sync test file exists" else echo "Combined sync test file is missing" fi
45-47: LGTM! New step for synchronous CJS imports added.This new step aligns well with the PR objective of testing synchronous imports. The use of
if: ${{ always() }}ensures comprehensive testing.Let's verify the existence of the new test file:
✅ Verification successful
CJS sync test file confirmed.
The file
smoke-tests/smoke-test-cjs-sync.cjsexists, ensuring that the new workflow step for synchronous CJS imports is properly supported.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of the new CJS sync test file if [ -f "smoke-tests/smoke-test-cjs-sync.cjs" ]; then echo "CJS sync test file exists" else echo "CJS sync test file is missing" fiLength of output: 114
Script:
#!/bin/bash # Verify the existence of the intended CJS sync test file if [ -f "smoke-tests/smoke-test-cjs-sync.cjs" ]; then echo "CJS sync test file exists" else echo "CJS sync test file is missing" fiLength of output: 109
package.json (3)
55-55: Approved: jiti updated to v2.3.3The jiti dependency has been updated from v1.21.6 to v2.3.3, which is a major version bump. This update aligns with the PR objective of transitioning to asynchronous imports supported by Jiti 2.
Please review the jiti changelog for any breaking changes or new features that may need to be addressed in the codebase. Run the following script to check for jiti usage that might need updating:
#!/bin/bash # Description: Check for jiti usage that might need updating # Test: Search for jiti imports and usage echo "Checking jiti usage:" rg --type typescript 'jiti' # Test: Search for synchronous import patterns that might need to be updated echo "Checking for potential synchronous imports to update:" rg --type typescript '(require|import.*from)'
47-55: Summary: Significant version updatesThis PR includes several important version updates:
- Node.js minimum version increased to v18
- cosmiconfig peer dependency updated to >=9
- TypeScript peer dependency updated to >=5
- jiti dependency updated to v2.3.3
These changes align with the PR objectives and modernize the package dependencies.
To ensure the stability of the package with these updates, please:
- Update the changelog to reflect these changes, especially the Node.js version requirement.
- Conduct comprehensive testing, including integration tests with the latest versions of cosmiconfig and TypeScript.
- Review and update any documentation that may be affected by these changes.
Run the following script to identify files that might need updating based on these changes:
#!/bin/bash # Description: Identify files that might need updating due to dependency changes # Test: Search for files mentioning Node.js version echo "Files potentially affected by Node.js version change:" rg --type markdown --type yaml 'node.*v1[6-7]' # Test: Search for files mentioning cosmiconfig or TypeScript versions echo "Files potentially affected by cosmiconfig or TypeScript version changes:" rg --type markdown --type yaml '(cosmiconfig.*8|typescript.*4)' # Test: Search for files mentioning jiti echo "Files potentially affected by jiti update:" rg --type markdown --type typescript 'jiti'
51-52: Approved: Updated peer dependenciesThe minimum versions for cosmiconfig and TypeScript have been increased:
- cosmiconfig: >=8.2 → >=9
- TypeScript: >=4 → >=5
These updates align with the latest major versions of these dependencies.
Please ensure that the codebase is compatible with these new versions. Run the following script to check for any usage of deprecated or changed APIs:
✅ Verification successful
Please run the following updated scripts to verify compatibility with the new dependency versions:
Verified: Updated peer dependencies
The updates to the minimum versions for cosmiconfig and TypeScript have been verified and are compatible with the current codebase:
- cosmiconfig: >=8.2 → >=9
- TypeScript: >=4 → >=5
No compatibility issues were detected with the new versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential compatibility issues with updated dependencies # Test: Search for cosmiconfig usage echo "Checking cosmiconfig usage:" rg --type typescript 'cosmiconfig' # Test: Search for TypeScript-specific syntax that might have changed in v5 echo "Checking for TypeScript v5 compatibility:" rg --type typescript '(satisfies|const enum|import type)'Length of output: 408
Script:
#!/bin/bash # Description: Check for potential compatibility issues with updated dependencies # Test: Search for cosmiconfig usage in TypeScript files echo "Checking cosmiconfig usage:" rg 'cosmiconfig' --glob '*.ts' --glob '*.tsx' # Test: Search for TypeScript v5 specific syntax in TypeScript files echo "Checking for TypeScript v5 compatibility:" rg '(satisfies|const enum|import type)' --glob '*.ts' --glob '*.tsx'Length of output: 1036
README.md (3)
Line range hint
11-37: LGTM! Important update to usage instructions.The changes correctly reflect the transition to asynchronous imports, which is a key objective of the PR. The addition of
awaitwhen loading configurations is crucial for users to understand the new usage pattern.
53-53: Correct update to simplified usage example.The addition of
awaitin this simplified example is consistent with the previous changes and reinforces the new asynchronous loading pattern.
Line range hint
11-72: Overall, excellent updates to the README.The changes to the README effectively communicate the key aspects of the upgrade to Jiti 2:
- The transition to asynchronous imports is clearly explained and demonstrated.
- The deprecation of synchronous loading is properly addressed, with guidance for users who still need it.
- Code examples are updated to reflect the new usage patterns.
These updates will greatly help users understand and implement the changes required by the upgrade.
lib/index.spec.ts (4)
1-1: Verify the necessity of the eslint-disable comment.The added eslint-disable comment for
@typescript-eslint/no-deprecatedsuggests the use of deprecated APIs in the tests. While this might be intentional for testing purposes, it's important to ensure that we're not inadvertently using deprecated features in the actual implementation.Could you please confirm if this eslint-disable is necessary? If so, consider adding a comment explaining why it's needed to improve code maintainability.
7-7: LGTM: Import statement updated correctly.The import statement has been appropriately updated to include
TypeScriptLoaderSync. This change aligns with the introduction of the new synchronous loader and enables its use in the test suite.
Line range hint
1-91: Overall assessment: Changes look good and align with PR objectives.The introduction of
TypeScriptLoaderSyncand the corresponding test updates are well-implemented. The changes maintain consistency between synchronous and asynchronous operations while introducing the new functionality. The test coverage appears to be comprehensive, testing both valid and invalid scenarios for both synchronous and asynchronous loaders.A few minor suggestions were made to improve code clarity and maintainability:
- Consider adding a comment explaining the necessity of the eslint-disable directive.
- The test cases for checking the exported loader functions could be combined for conciseness.
Please address these suggestions if you find them beneficial. Otherwise, the changes are approved and ready for merging.
24-24: LGTM: TypeScriptLoaderSync correctly used in synchronous tests.The changes appropriately use
TypeScriptLoaderSync()for synchronous tests while maintainingTypeScriptLoader()for asynchronous tests. This ensures that the correct loader is used for each test scenario.To ensure consistency across the codebase, let's verify that
TypeScriptLoaderSyncis used for all synchronous operations andTypeScriptLoaderfor asynchronous ones:Please review the output to confirm that the loaders are used consistently throughout the codebase.
Also applies to: 39-39
✅ Verification successful
Verified: TypeScriptLoaderSync is consistently used in all synchronous tests.
The shell script output confirms that
TypeScriptLoaderSync()is appropriately utilized in all synchronous test scenarios, whileTypeScriptLoader()remains correctly used for asynchronous tests throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct usage of TypeScriptLoader and TypeScriptLoaderSync # Test: Search for TypeScriptLoader usage echo "TypeScriptLoader usage:" rg --type typescript "TypeScriptLoader\(\)" -C 3 echo "\nTypeScriptLoaderSync usage:" rg --type typescript "TypeScriptLoaderSync\(\)" -C 3Length of output: 301
Script:
#!/bin/bash # Description: Verify correct usage of TypeScriptLoader and TypeScriptLoaderSync in .ts and .tsx files # Search for TypeScriptLoader usage echo "TypeScriptLoader usage in .ts and .tsx files:" rg "TypeScriptLoader\(\)" -g "*.ts" -g "*.tsx" -C 3 # Search for TypeScriptLoaderSync usage echo -e "\nTypeScriptLoaderSync usage in .ts and .tsx files:" rg "TypeScriptLoaderSync\(\)" -g "*.ts" -g "*.tsx" -C 3Length of output: 3001
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (12)
smoke-tests/smoke-test-esm.mjs (1)
23-31: Improved error handling looks great!The enhanced error handling provides more specific and helpful error messages, which is excellent for debugging. The separation of different error types (TypeError, SyntaxError, and others) allows for more targeted error reporting.
Consider using template literals for better readability and consistency:
- console.error("Type error occurred:", error.message); + console.error(`Type error occurred: ${error.message}`); - console.error("Syntax error in configuration file:", error.message); + console.error(`Syntax error in configuration file: ${error.message}`); - console.error("An unexpected error occurred:", error.message); + console.error(`An unexpected error occurred: ${error.message}`); - console.debug("Error stack trace:", error.stack); + console.debug(`Error stack trace: ${error.stack}`);smoke-tests/smoke-test-cjs.cjs (1)
23-31: Improved error handling, but could be more conciseThe enhanced error handling is a good improvement, providing more specific information about the type of error encountered. This will help in debugging and troubleshooting.
However, consider the following suggestions to make it even better:
The error messages could be more concise. For example:
console.error(`CJS configuration load failed: ${error instanceof TypeError ? 'Type error' : error instanceof SyntaxError ? 'Syntax error' : 'Unexpected error'} - ${error.message}`);Consider adding specific actions or recovery steps based on the error type, if applicable.
You might want to use
console.errorfor the stack trace instead ofconsole.debug, as it's typically used for critical debugging information.smoke-tests/smoke-test.js (2)
15-20: LGTM! Consider adding a more comprehensive mock config.The change to asynchronous invocation aligns well with the PR objectives. Good job on updating the smoke test to use
awaitfor both ESM and CJS loaders.Consider replacing the empty mock config with a more comprehensive one to test the loaders with realistic input. For example:
const mockConfig = { loaderOptions: { transpileOnly: true, compilerOptions: { module: 'esnext', }, }, };Then use
mockConfigin both loader calls:const esmResult = await esm(mockConfig); const cjsResult = await cjs(mockConfig);
27-31: Excellent addition of type checking for the CJS loader result!This assertion complements the ESM loader check, ensuring consistency across both module systems. It's a valuable addition that strengthens the smoke test.
For consistency, consider using the same casing for "function" in both error messages:
assert.strictEqual( typeof cjsResult, "function", "CJS loader should return a function" );This makes it match the ESM error message and maintains a consistent style throughout the test.
lib/loader.ts (3)
10-27: LGTM! TypeScriptLoader function improved with minor suggestion.The changes to the
TypeScriptLoaderfunction are appropriate and address previous concerns:
- The function signature now correctly returns
LoaderAsync.- The implementation uses
createJiti, aligning with the import changes.- Error handling has been improved with
TypeScriptCompileError.- The function now accepts both
filepathandcontentparameters, addressing the previous mismatch issue.Consider renaming the unused
_contentparameter to_to explicitly indicate it's not used:- return async (path: string, _content: string): Promise<LoaderResult> => { + return async (path: string, _: string): Promise<LoaderResult> => {
29-34: LGTM! TypeScriptLoaderSync function added with suggestion.The addition of the
TypeScriptLoaderSyncfunction is appropriate:
- It provides backwards compatibility while being marked as deprecated.
- The implementation is consistent with the asynchronous version.
- The deprecation notice aligns with the PR objective of transitioning to asynchronous imports.
Consider updating the function signature to match the
LoaderSynctype exactly:- return (path: string, _content: string): LoaderResult => { + return (filepath: string, content: string): LoaderResult => {This change would make the function signature consistent with the
LoaderSynctype definition and the asynchronous version.
Line range hint
35-47: LGTM! TypeScriptLoaderSync function body with minor suggestion.The implementation of the
TypeScriptLoaderSyncfunction body is appropriate:
- It correctly uses the synchronous
loader(path)call.- Error handling is consistent with the asynchronous version, improving debugging experience.
Consider renaming the unused
_contentparameter to_to explicitly indicate it's not used:- return (path: string, _content: string): LoaderResult => { + return (path: string, _: string): LoaderResult => {This change would make it clear that the
contentparameter is intentionally unused in the current implementation..github/workflows/build.yml (1)
45-53: Excellent addition of synchronous import tests!The new steps for testing synchronous imports in CJS, ESM, and combined formats significantly enhance the workflow's test coverage. These additions align well with the PR objectives of upgrading to Jiti 2 and supporting both synchronous and asynchronous imports.
The consistent use of
if: ${{ always() }}ensures these tests run regardless of previous step outcomes, maintaining the workflow's robustness. This comprehensive testing approach will help catch any potential issues related to the Jiti 2 upgrade across different module formats.Consider adding a comment in the workflow file explaining the purpose of these new synchronous import tests, especially in relation to the Jiti 2 upgrade. This will help future maintainers understand the rationale behind these additions.
lib/loader.spec.ts (4)
21-21: LGTM: Improved test setup and teardownThe changes to use
beforeEachinstead ofbeforeAll, along with the addition ofafterEachto restore mocks, ensure a clean state for each test. This is a good practice that improves test isolation.Consider moving the
jitiCreateJitiSpydeclaration inside thebeforeEachblock for better encapsulation:beforeEach(() => { jitiCreateJitiSpy = jest.spyOn(jiti, "createJiti"); });This would eliminate the need for the separate declaration and improve code organization.
Also applies to: 27-32
42-66: LGTM: Improved async test casesThe updates to the test cases using async/await syntax and the simplified error handling with
expect(...).rejects.toThrow()align well with modern asynchronous testing practices. The new test case for checking jiti instance reuse is a valuable addition.In the test "should fail on parsing an invalid TS file" (lines 47-52), consider being more specific about the expected error:
await expect( loader(filePath, readFixtureContent(filePath)) ).rejects.toThrow(TypeScriptCompileError);This would make the test more robust by ensuring that the specific
TypeScriptCompileErroris thrown, rather than any error.
68-93: LGTM: Comprehensive jiti error handling testThe addition of a nested describe block for jiti-specific tests and the test case for rethrowing non-Error instances improve the error handling coverage. This ensures that unexpected errors from jiti are properly handled.
Consider replacing the use of
fail()with a more idiomatic Jest approach:await expect(async () => { await loader("filePath", "invalidInput"); }).rejects.toStrictEqual(unknownError);This approach is more concise and aligns better with Jest's testing style.
130-153: LGTM: Consistent sync jiti error handling testThe addition of a nested describe block for jiti-specific tests in the synchronous suite and the test case for rethrowing non-Error instances maintain consistency with the asynchronous suite. This ensures that error handling is thoroughly tested in both contexts.
For consistency with the suggested improvement in the async suite, consider updating the error checking approach:
expect(() => { loader("filePath", "invalidInput"); }).toThrow(unknownError);This change would align the synchronous test more closely with Jest's idiomatic testing style and make it consistent with the async suite.
📜 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 (13)
- .github/workflows/build.yml (1 hunks)
- README.md (3 hunks)
- lib/index.spec.ts (3 hunks)
- lib/index.ts (1 hunks)
- lib/loader.spec.ts (1 hunks)
- lib/loader.ts (1 hunks)
- package.json (1 hunks)
- smoke-tests/smoke-test-cjs-sync.cjs (1 hunks)
- smoke-tests/smoke-test-cjs.cjs (1 hunks)
- smoke-tests/smoke-test-esm-sync.mjs (1 hunks)
- smoke-tests/smoke-test-esm.mjs (1 hunks)
- smoke-tests/smoke-test-sync.js (1 hunks)
- smoke-tests/smoke-test.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- README.md
- lib/index.spec.ts
- lib/index.ts
- package.json
- smoke-tests/smoke-test-cjs-sync.cjs
- smoke-tests/smoke-test-esm-sync.mjs
- smoke-tests/smoke-test-sync.js
🧰 Additional context used
🔇 Additional comments (10)
smoke-tests/smoke-test.js (2)
22-26: Great addition of type checking for the ESM loader result!This assertion enhances the smoke test by ensuring that the ESM loader returns a function as expected. It's a valuable addition that aligns well with the PR objectives.
15-31: Overall, excellent updates to the smoke test!The changes in this file significantly improve the robustness of the smoke test:
- The loaders are now invoked asynchronously, aligning with the PR objectives.
- Type checking has been added for both ESM and CJS loader results.
- The test now more accurately reflects the expected usage of the loaders.
These updates contribute to a more reliable test suite and help ensure the correct functionality of the TypeScriptLoader in both ESM and CJS environments.
lib/loader.ts (2)
1-5: LGTM! Import statements and type definitions improved.The changes in import statements and type definitions are appropriate:
- Importing from 'cosmiconfig' and 'jiti' instead of internal paths improves maintainability.
- New type definitions for
JitiandJitiOptionsenhance type safety.These changes effectively address the previous concern about importing from internal module paths.
8-9: LGTM! LoaderAsync type definition added.The addition of the
LoaderAsynctype is appropriate:
- It correctly defines the signature for an asynchronous loader function.
- This type aligns with the changes in the
TypeScriptLoaderfunction, improving type safety and code clarity..github/workflows/build.yml (3)
51-53: LGTM! New combined synchronous CJS and ESM import test added.This step provides comprehensive coverage by testing synchronous imports for both CJS and ESM in a single test. The configuration aligns with the existing workflow structure.
To ensure the new combined test file exists and includes both CJS and ESM imports, run the following script:
#!/bin/bash # Description: Verify the existence and basic structure of the new combined sync test file. # Test: Check if the file exists and contains both require and import statements if [ -f "smoke-tests/smoke-test-sync.js" ]; then echo "File exists. Checking content:" echo "CJS imports:" grep -n "require(" smoke-tests/smoke-test-sync.js echo "ESM imports:" grep -n "import" smoke-tests/smoke-test-sync.js else echo "Error: smoke-tests/smoke-test-sync.js does not exist." fi
48-50: LGTM! New synchronous ESM import test added.This step complements the CJS test by adding coverage for synchronous imports in ESM format. The configuration is consistent with other test steps.
To ensure the new ESM test file exists and is properly implemented, run the following script:
#!/bin/bash # Description: Verify the existence and basic structure of the new ESM sync test file. # Test: Check if the file exists and contains basic import statement if [ -f "smoke-tests/smoke-test-esm-sync.mjs" ]; then echo "File exists. Checking content:" grep -n "import" smoke-tests/smoke-test-esm-sync.mjs else echo "Error: smoke-tests/smoke-test-esm-sync.mjs does not exist." fi
45-47: LGTM! New synchronous CJS import test added.The addition of this step enhances the test coverage by including synchronous imports for CJS. The step is correctly configured to always run, maintaining consistency with other test steps.
To ensure the new test file exists and is properly implemented, run the following script:
✅ Verification successful
Verification Successful: Synchronous CJS import test exists and is correctly implemented.
The new test file
smoke-tests/smoke-test-cjs-sync.cjsexists and contains the necessaryrequirestatements, confirming proper implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and basic structure of the new CJS sync test file. # Test: Check if the file exists and contains basic import statement if [ -f "smoke-tests/smoke-test-cjs-sync.cjs" ]; then echo "File exists. Checking content:" grep -n "require(" smoke-tests/smoke-test-cjs-sync.cjs else echo "Error: smoke-tests/smoke-test-cjs-sync.cjs does not exist." fiLength of output: 596
Script:
#!/bin/bash # Description: Verify the existence and basic structure of the new CJS sync test file. # Test: Check if the file exists and contains a 'require' statement if [ -f "smoke-tests/smoke-test-cjs-sync.cjs" ]; then echo "File exists. Checking content:" grep -n "require(" smoke-tests/smoke-test-cjs-sync.cjs else echo "Error: smoke-tests/smoke-test-cjs-sync.cjs does not exist." fiLength of output: 336
lib/loader.spec.ts (3)
4-4: LGTM: Import and mock updatesThe changes to the import statements and the
jitimock are appropriate for the transition to asynchronous testing and the new loader structure. The use ofLoaderResultandLoaderSynctypes, along with spying oncreateJiti, provides better type safety and control in the tests.Also applies to: 7-7, 15-15
35-40: LGTM: Well-structured async test suiteThe addition of a separate describe block for asynchronous tests and the setup of the loader in a
beforeEachblock within this suite is a good practice. It clearly separates async and sync tests, improving readability and ensuring a fresh loader instance for each test.
1-156: Overall assessment: Comprehensive and well-structured test suite updateThe changes to
lib/loader.spec.tsrepresent a significant improvement in test coverage and alignment with modern testing practices. Key points:
- Clear separation of async and sync test suites improves readability and maintainability.
- Consistent structure between async and sync suites facilitates understanding and future updates.
- Improved error handling and jiti instance management enhance the robustness of the tests.
- The transition towards async patterns is evident, with appropriate handling of the deprecated sync loader.
These updates will greatly contribute to the reliability and maintainability of the codebase. Great job on the refactoring!
This upgrade moves to use the async imports supported by Jiti, marking the synchronous importing of config files deprecated.
See: await jiti.import()`
Summary by CodeRabbit
Release Notes
New Features
TypeScriptLoaderSyncfor synchronous loading of TypeScript configurations.Documentation
awaitconfiguration loading and introduced a section on the deprecated synchronous loader.Bug Fixes
Chores
package.jsondependencies and Node.js version requirements.