Improve the update CLI command and fix missing tsconfig.json error #1315
Conversation
🦋 Changeset detectedLatest commit: fcf34c9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduced in this pull request focus on improving the development experience by enhancing error handling, modifying command behaviors, and adding new configuration options. Key updates include a mechanism to gracefully handle missing Changes
Possibly related PRs
Suggested reviewers
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 (
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- .changeset/clever-buses-watch.md (1 hunks)
- .changeset/soft-ladybugs-promise.md (1 hunks)
- packages/cli-v3/src/commands/dev.ts (1 hunks)
- packages/cli-v3/src/commands/login.ts (2 hunks)
- packages/cli-v3/src/commands/update.ts (5 hunks)
- packages/cli-v3/src/commands/whoami.ts (2 hunks)
- packages/cli-v3/src/config.ts (3 hunks)
- packages/cli-v3/src/utilities/sourceFiles.ts (1 hunks)
- packages/core/src/v3/build/resolvedConfig.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/cli-v3/src/utilities/sourceFiles.ts
Additional comments not posted (14)
.changeset/soft-ladybugs-promise.md (1)
1-5: Changeset looks good!The changeset clearly describes the purpose and impact of the changes made to the
trigger.devCLI tool. The fixes for the update command and the modification to hide the "whoami" command output in dev mode seem to be focused on improving the user experience and streamlining the development workflow..changeset/clever-buses-watch.md (1)
1-6: LGTM!The changeset file looks good. The metadata and description align with the PR objectives and the AI-generated summary.
packages/core/src/v3/build/resolvedConfig.ts (1)
27-27: LGTM!The addition of the optional
tsconfigPathproperty to theResolvedConfigtype is a valid enhancement. It allows specifying a path to a TypeScript configuration file, which can be useful for projects that utilize TypeScript in their build processes. The change expands the configurability of theResolvedConfigtype without breaking existing code.packages/cli-v3/src/commands/whoami.ts (1)
54-55: LGTM!The addition of the
silentparameter and the corresponding conditional checks for controlling the loading spinner messages are implemented correctly. This change enhances the flexibility of thewhoAmIfunction by allowing it to operate in a silent mode when needed, without introducing any apparent issues.Also applies to: 63-65, 71-71, 74-79, 118-118
packages/cli-v3/src/commands/dev.ts (1)
54-54: LGTM! Thesilentparameter provides flexibility in controlling login output verbosity.The addition of the
silent: trueparameter to theloginfunction call allows for suppressing the output typically displayed during the login operation. This can be beneficial in scenarios where a quieter execution is desired, such as in automated scripts or background processes.However, please ensure that critical error messages or important login-related information is still communicated to the user when necessary, even in silent mode. This will help maintain a balance between verbosity control and providing essential feedback to the user.
packages/cli-v3/src/config.ts (2)
141-141: LGTM!The change to use the new
safeResolveTsConfigfunction instead ofresolveTSConfigis a good improvement. It will handle potential errors more gracefully during the resolution of the TypeScript configuration file.
191-197: Great addition of thesafeResolveTsConfigfunction!The
safeResolveTsConfigfunction is a nice addition to handle potential errors gracefully when resolving the TypeScript configuration file. Wrapping theresolveTSConfigfunction in a try-catch block and returningundefinedif an error occurs is a good approach to improve the robustness of the configuration resolution process.This change aligns with the AI-generated summary and enhances the overall error handling in the codebase.
packages/cli-v3/src/commands/login.ts (3)
62-62: LGTM!The new optional
silentproperty is correctly added to theLoginOptionstype to control the verbosity of the login command. Making it optional ensures backward compatibility.
68-73: Looks good!The
optsobject is correctly defined with default values, includingsilentset tofalse. Spreading theoptionsobject allows the caller to override these defaults. This ensures that the login command remains verbose by default unless explicitly silenced.
120-121: Verify thewhoAmIfunction.Please ensure that:
- The
whoAmIfunction signature is updated to accept the newsilentargument.- The
whoAmIfunction implementation handles thesilentargument correctly to control its verbosity.Run the following script to verify the
whoAmIfunction:packages/cli-v3/src/commands/update.ts (4)
130-130: LGTM!The change from "Nothing to do" to "Nothing to update" improves clarity and aligns with the action being performed.
Line range hint
136-209: LGTM!The updated error handling and user confirmation logic based on the
embeddedflag enhances the user experience by providing appropriate messages and actions in different scenarios.
46-46: Verify the impact of passingfalsefor theembeddedflag.The change looks good, but please ensure that explicitly passing
falseas theembeddedflag toupdateTriggerPackagesaligns with the expected behavior and doesn't introduce any unintended side effects.Run the following script to analyze the usage of the
embeddedflag inupdateTriggerPackages:Verification successful
To continue our verification process, let's search for the implementation of
updateTriggerPackagesand analyze how it uses theembeddedparameter.
To complete our verification, we need to examine how the
embeddedparameter is used within theupdateTriggerPackagesfunction. Let's search for its usage.
The change is safe and doesn't impact the behavior of
updateTriggerPackages.After thorough investigation, we can confirm that explicitly passing
falsefor theembeddedflag inupdateCommanddoes not affect the execution ofupdateTriggerPackages. Theembeddedparameter is not used within the function body, suggesting it might be a remnant from previous versions or reserved for future use.Consider removing the
embeddedparameter from theupdateTriggerPackagesfunction signature if it's not used elsewhere in the codebase, as this would improve code clarity.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the usage of the `embedded` flag in `updateTriggerPackages`. # Test: Search for conditional logic based on the `embedded` flag. Expect: Relevant code snippets. rg --type typescript -A 5 $'if \(embedded\)' $(fd -e ts -E node_modules -E dist -E build -E coverage -E .git)Length of output: 42758
Script:
#!/bin/bash # Search for the implementation of updateTriggerPackages rg -A 10 -B 5 "function updateTriggerPackages" packages/cli-v3/src/Length of output: 1163
Script:
#!/bin/bash # Search for the usage of 'embedded' within updateTriggerPackages rg -A 10 -B 10 "function updateTriggerPackages" packages/cli-v3/src/commands/update.ts | rg "embedded"Length of output: 124
Line range hint
220-259: Verify the impact of silent package installation.The changes to the spinner messages improve clarity and provide more context to the user. However, please ensure that running the package installation silently at line 254 doesn't hide any important information or errors that the user should be aware of.
Run the following script to analyze the impact of silent package installation:
Verification successful
Silent installation is handled with appropriate error messaging.
The
installDependenciesfunction is called withsilent: true, but it's wrapped in a try-catch block. If an error occurs during installation, a failure message is displayed to the user. Additionally, there's anexitHandlermechanism that seems to handle cleanup or warnings about manual rollbacks.While the silent installation might hide some information during the process, the error handling ensures that users are informed if something goes wrong. The implementation strikes a balance between a clean user experience and necessary error reporting.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the impact of silent package installation. # Test: Search for error handling logic related to package installation. Expect: Relevant code snippets. rg --type typescript -A 10 $'installDependencies\(' $(fd -e ts -E node_modules -E dist -E build -E coverage -E .git)Length of output: 42765
Script:
#!/bin/bash # Search for installDependencies function and its usage rg -i "function\s+installDependencies|installDependencies\s*\(" packages/cli-v3/src # Search for error handling related to package installation rg -i "catch.*install|error.*install" packages/cli-v3/srcLength of output: 506
Script:
#!/bin/bash # Check the implementation of installDependencies rg -A 10 "function\s+installDependencies" packages/cli-v3/src # Check usage of parseNpmInstallError rg "parseNpmInstallError" packages/cli-v3/src # Check error handling around installDependencies in update.ts rg -A 10 -B 5 "installDependencies" packages/cli-v3/src/commands/update.tsLength of output: 1588
| }, | ||
| } | ||
| ); | ||
| ) as ResolvedConfig; // TODO: For some reason, without this, there is a weird type error complaining about tsconfigPath being string | nullish, which can't be assigned to string | undefined |
There was a problem hiding this comment.
Investigate the root cause of the type error instead of using a type assertion.
The type assertion is used to suppress a type error related to the tsconfigPath property. However, it's better to investigate and fix the root cause of the type mismatch instead of relying on a type assertion.
Please consider the following:
- Analyze why
tsconfigPathis being treated asstring | nullishinstead ofstring | undefined. - Ensure that the type definitions for
tsconfigPathare consistent throughout the codebase. - Consider updating the type definitions or the logic that assigns the value to
tsconfigPathto resolve the type mismatch.
Using a type assertion may hide potential issues and make the code less maintainable in the long run.
commit: |
…output when running in dev
ericallam
force-pushed
the
v3/cli-update-fixes
branch
from
126c4bd to
fcf34c9
Compare
updatecommand, it now no longer shows "mismatch" errors when using in non-embedded mode.Summary by CodeRabbit
New Features
trigger.devCLI tool for better reliability and user experience, including a silent mode for commands.whoamicommand and login process with optional silent operation.Bug Fixes