feat: full ESM config support using Ziti (build #1041) #1048
Conversation
WalkthroughThis pull request updates dependency management and refines configuration file loading across several projects. In multiple package manifests, TypeScript-related dependencies ( Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
🪧 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 (
|
sorccu
changed the title
![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)
examples/boilerplate-project/package.json (1)
2-2: Typo in package nameThere's a typo in the package name: "boilerlate-project" should be "boilerplate-project".
- "name": "boilerlate-project", + "name": "boilerplate-project",packages/cli/src/playwright/playwright-config-loader.ts (1)
15-16: Consider logging if no config file was found.While returning
undefinedclearly indicates that a config file was not found, providing a small log or debug message here could be beneficial to inform users why the config is missing. This is just a minor improvement for debugging.Also applies to: 19-20, 22-22
packages/cli/src/services/checkly-config-loader.ts (1)
112-114: Early file existence check is convenient.Skipping non-existent files before attempting to load them prevents unnecessary errors. Consider logging or warning if multiple config files could be found concurrently, to guide users on potential configuration conflicts.
packages/create-cli/src/utils/fileloader.ts (1)
36-37: Use optional chaining for safer property access.As hinted by static analysis, consider using
err.message?.includes('Unable to compile TypeScript')to avoid potential runtime errors whenerr.messageis undefined.- if (err.message && err.message.contains('Unable to compile TypeScript')) { + if (err.message?.includes('Unable to compile TypeScript')) {🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
examples/advanced-project/package.json(1 hunks)examples/boilerplate-project/package.json(1 hunks)packages/cli/package.json(1 hunks)packages/cli/src/playwright/playwright-config-loader.ts(1 hunks)packages/cli/src/services/__tests__/checkly-config-loader.spec.ts(1 hunks)packages/cli/src/services/checkly-config-loader.ts(3 hunks)packages/cli/src/services/project-parser.ts(2 hunks)packages/cli/src/services/util.ts(3 hunks)packages/create-cli/e2e/__tests__/fixtures/initiated-project/package.json(1 hunks)packages/create-cli/e2e/__tests__/fixtures/playwright-project/package.json(1 hunks)packages/create-cli/package.json(1 hunks)packages/create-cli/src/actions/dependencies.ts(1 hunks)packages/create-cli/src/actions/playwright.ts(1 hunks)packages/create-cli/src/utils/__tests__/fixtures/checkly-project/package.json(1 hunks)packages/create-cli/src/utils/directory.ts(1 hunks)packages/create-cli/src/utils/fileloader.ts(3 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
packages/create-cli/src/utils/fileloader.ts (1)
packages/cli/src/services/util.ts (1)
loadFile(53-68)
packages/cli/src/services/util.ts (1)
packages/create-cli/src/utils/fileloader.ts (1)
loadFile(3-18)
🪛 Biome (1.9.4)
packages/create-cli/src/utils/fileloader.ts
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/cli/src/services/util.ts
[error] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test - windows-latest
- GitHub Check: test - ubuntu-latest
🔇 Additional comments (30)
examples/advanced-project/package.json (1)
14-14: Good dependency update to support ESM modulesReplacing TypeScript tools (ts-node and typescript) with jiti is a good approach for enabling better ESM module support. Jiti acts as a runtime that can handle both TypeScript and ESM modules, which aligns well with the PR's goal of providing full ESM config support.
examples/boilerplate-project/package.json (1)
14-14: Good dependency update to support ESM modulesThe addition of jiti as a replacement for ts-node and typescript follows the same pattern as other package.json files in this PR. This change will enable better ESM module support across example projects.
packages/cli/src/services/__tests__/checkly-config-loader.spec.ts (1)
26-26: Good expansion of supported config file extensionsThe error message now includes additional file extensions (.mts, .cts, .cjs) that are now supported. This properly reflects the expanded config file format capabilities and is an important part of providing full ESM support. The .mts extension in particular is crucial for TypeScript ESM modules.
packages/create-cli/e2e/__tests__/fixtures/initiated-project/package.json (1)
13-13: Good dependency update for test fixturesReplacing ts-node and typescript with jiti in test fixtures ensures that the test environment mirrors the expected real-world project structure. This change maintains consistency with the other package.json updates in this PR and properly tests the new dependency arrangement for ESM support.
packages/create-cli/src/actions/dependencies.ts (1)
15-18: LGTM: Dependency update aligns with project-wide changesThe change from TypeScript-related tools (
ts-node,typescript) to usingjitiis consistent with the broader project strategy. The version constraint^2follows semantic versioning standards.packages/create-cli/src/utils/__tests__/fixtures/checkly-project/package.json (1)
12-15: LGTM: Test fixture updated to match new dependency strategyThe fixture now correctly reflects the project's move from TypeScript-specific dependencies to using
jitias a runtime loader, maintaining consistency with the changes in the main code.packages/create-cli/e2e/__tests__/fixtures/playwright-project/package.json (1)
11-14: LGTM: Playwright test fixture updated correctlyThis change properly aligns the Playwright project test fixture with the project's new dependency approach, consistently using
jiti ^2instead of TypeScript-specific dependencies.packages/create-cli/src/actions/playwright.ts (1)
47-55: LGTM: Enhanced configuration file supportGood enhancement to support additional module system file extensions. The function now properly handles
.mts,.cts, and.cjsconfiguration files, increasing flexibility for users with different module preferences.packages/create-cli/src/utils/directory.ts (1)
27-35: Improved error handling and file extension supportThis change enhances the
loadPlaywrightConfigfunction with proper existence checks and expanded file extension support. The regex/\.[mc]?(js|ts)$/now handles ESM and CommonJS variants, supporting.js,.ts,.mjs,.cjs,.mts, and.ctsfiles.packages/create-cli/package.json (2)
83-83: LGTM - Adding jiti as a dev dependencyAdding jiti aligns with the PR's goals to support ESM configs.
89-96: Good approach making jiti an optional peer dependencyMaking jiti an optional peer dependency with
>=2version constraint provides flexibility while ensuring compatibility when it's present.packages/cli/package.json (2)
116-116: LGTM - Adding jiti as a dev dependencyAdding jiti aligns with the PR's goals to support ESM configs.
124-131: Good approach making jiti an optional peer dependencyMaking jiti an optional peer dependency with
>=2version constraint provides flexibility while ensuring compatibility when it's present.packages/cli/src/services/project-parser.ts (2)
3-3: LGTM - Updated import to use consolidated file loaderImporting the unified
loadFilefunction instead of separate loaders simplifies the code.
88-93: Improved code with unified file loading approachThe updated implementation:
- Simplifies logic by using a single regex for all supported file types
- Uses a unified
loadFilefunction instead of separate functions for JS and TS- Adds support for additional file extensions (
.mjs,.cjs,.mts,.cts)This change improves maintainability while expanding format support.
packages/cli/src/playwright/playwright-config-loader.ts (2)
2-3: Good import consolidation.Consolidating filesystem and utility imports at the top helps keep the code more organized.
6-13: Support for multiple config file extensions is a robust addition.Including
.mts,.cts,.mjs, and.cjsoffers extra flexibility for Playwright configuration.packages/cli/src/services/checkly-config-loader.ts (3)
3-3: Centralized file loading is a good refactor.Using
loadFilefrom./utilensures consistent file-loading logic across the codebase and removes duplicated code.
80-88: Expanding configuration file extensions increases compatibility.Adding
.mts,.cts, and.cjsprovides more ways for users to structure their Checkly config.
107-107: Defaults maintain clarity and ease-of-use.Introducing new default filenames in the
loadChecklyConfigparameters is straightforward and meshes well with the broader multi-extension support.packages/create-cli/src/utils/fileloader.ts (6)
3-11: Refined file loading logic enhances modularity.Using a single
loadFilefunction that dynamically branches for TS or JS simplifies future maintenance and reduces duplicated code.
20-26:loadTsFileDefaultis a clean abstraction.Centralizing the TypeScript-specific loading path via
jitior fallback options is a great approach to handle different TS setups.
28-40: Fallback tots-nodeensures backward compatibility.This logic rightly handles scenarios where
jitiis not installed, usingts-nodeas a graceful fallback.🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
48-53: Type alias for Jiti usage is well structured.Storing the
Jititype here is clear for future reference if more Jiti-based functionality is added.
55-67:getJition demand is performance-friendly.Lazily importing and caching Jiti avoids overhead for users who are not using TypeScript. This design is sensible.
71-90:getTsNodeServicegracefully accommodates TS projects without jiti.The fallback logic is robust, and the code re-enables then disables the TS compiler as needed, minimizing side effects.
packages/cli/src/services/util.ts (4)
53-61: Streamlined file loader logic.This updated
loadFilefunction neatly distinguishes between TypeScript and other file extensions, and the fallback to call the default export if it's a function is well-handled. No issues detected here.
98-103: Advanced TypeScript pattern forJititype.Using a dynamically inferred return type here is acceptable given the complexity of the
jitiimport. This helps avoid direct type imports that cause issues in certain test environments.
105-118: Robust lazy loading of thejitimodule.This approach avoids introducing an explicit dependency for all users, and the error handling for missing
jitiis well-managed.
120-141: Selectivets-nodedynamic import for backward compatibility.This gated logic effectively avoids forcing
ts-nodeon all users. The error handling for the module not found scenario is correct and returnsundefinedgracefully.
| async function loadTsFileDefault (filepath: string): Promise<any> { | ||
| const jiti = await getJiti() | ||
| if (jiti) { | ||
| return jiti.import<any>(filepath, { | ||
| default: true, | ||
| }) | ||
| } | ||
|
|
||
| // Backward-compatibility for users who installed ts-node. | ||
| const tsCompiler = await getTsNodeService() | ||
| if (tsCompiler) { | ||
| tsCompiler.enabled(true) | ||
| let { default: exported } = await require(filepath) | ||
| if (exported instanceof Function) { | ||
| exported = await exported() | ||
| let exported: any | ||
| try { | ||
| exported = (await require(filepath)).default | ||
| } catch (err: any) { | ||
| if (err.message && err.message.contains('Unable to compile TypeScript')) { | ||
| throw new Error(`You might try installing "jiti" instead of "ts-node" for improved TypeScript support\n${err.stack}`) | ||
| } | ||
| throw err | ||
| } | ||
| tsCompiler.enabled(false) // Re-disable the TS compiler | ||
| return exported | ||
| } | ||
|
|
||
| throw new Error('Please install "jiti" to use TypeScript files') | ||
| } |
There was a problem hiding this comment.
Potential runtime error: String.prototype.contains is not standard JavaScript.
In the catch block at line 86, calling err.message.contains() may fail on environments that lack this method. Use the standard includes method instead:
- if (err.message && err.message.contains('Unable to compile TypeScript')) {
+ if (err.message && err.message.includes('Unable to compile TypeScript')) {📝 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.
| async function loadTsFileDefault (filepath: string): Promise<any> { | |
| const jiti = await getJiti() | |
| if (jiti) { | |
| return jiti.import<any>(filepath, { | |
| default: true, | |
| }) | |
| } | |
| // Backward-compatibility for users who installed ts-node. | |
| const tsCompiler = await getTsNodeService() | |
| if (tsCompiler) { | |
| tsCompiler.enabled(true) | |
| let { default: exported } = await require(filepath) | |
| if (exported instanceof Function) { | |
| exported = await exported() | |
| let exported: any | |
| try { | |
| exported = (await require(filepath)).default | |
| } catch (err: any) { | |
| if (err.message && err.message.contains('Unable to compile TypeScript')) { | |
| throw new Error(`You might try installing "jiti" instead of "ts-node" for improved TypeScript support\n${err.stack}`) | |
| } | |
| throw err | |
| } | |
| tsCompiler.enabled(false) // Re-disable the TS compiler | |
| return exported | |
| } | |
| throw new Error('Please install "jiti" to use TypeScript files') | |
| } | |
| async function loadTsFileDefault (filepath: string): Promise<any> { | |
| const jiti = await getJiti() | |
| if (jiti) { | |
| return jiti.import<any>(filepath, { | |
| default: true, | |
| }) | |
| } | |
| // Backward-compatibility for users who installed ts-node. | |
| const tsCompiler = await getTsNodeService() | |
| if (tsCompiler) { | |
| tsCompiler.enabled(true) | |
| let exported: any | |
| try { | |
| exported = (await require(filepath)).default | |
| } catch (err: any) { | |
| if (err.message && err.message.includes('Unable to compile TypeScript')) { | |
| throw new Error(`You might try installing "jiti" instead of "ts-node" for improved TypeScript support\n${err.stack}`) | |
| } | |
| throw err | |
| } | |
| tsCompiler.enabled(false) // Re-disable the TS compiler | |
| return exported | |
| } | |
| throw new Error('Please install "jiti" to use TypeScript files') | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
🎉 Experimental release successfully published on npm |
This PR is a temporary solution to help build #1041 with secrets enabled.