Skip to content
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.

Sign up for GitHub

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

Make update check faster #249

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Make update check faster #249

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3e3ee5a
Make update check faster
Oct 10, 2023
b9d4e9a
run checkForUpdates in isolate
Oct 10, 2023
0bd0153
rollback
Oct 10, 2023
bd6e76d
simplify
Nov 4, 2023
42c41c8
fix tests
Nov 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 38 additions & 26 deletions sidekick_core/lib/sidekick_core.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,8 +284,20 @@ class SidekickCommandRunner<T> extends CompletionCommandRunner<T> {
final unmount = mount(debugName: args.join(' '));

ArgResults? parsedArgs;
Future<void Function()?>? updateCheck;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Future<void> should be sufficient. The void Function() seems redundant

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated it to a String? :)


try {
parsedArgs = parse(args);

if (_isUpdateCheckEnabled &&
!_isSidekickCliUpdateCommand(parsedArgs) &&
!_isRunningTabCompletionCommand(parsedArgs)) {
// print warning if CLI update is available
// TODO prevent multiple update checks when a command start another command

updateCheck = _checkForUpdates();
}

if (parsedArgs['version'] == true) {
print('${SidekickContext.cliName} is using sidekick version $version');
return null;
Expand All @@ -294,23 +306,13 @@ class SidekickCommandRunner<T> extends CompletionCommandRunner<T> {
final result = await super.runCommand(parsedArgs);
return result;
} finally {
// don't print anything additionally when running the hidden tab completion command (runs in the background when pressing tab),
// otherwise anything that is printed will also be used as suggestion
bool isRunningTabCompletionCommand() {
final reservedCommands = [
HandleCompletionRequestCommand.commandName,
InstallCompletionFilesCommand.commandName,
];
return reservedCommands.contains(parsedArgs?.command?.name);
}

// don't show the install-global suggesting when running the install-global command
bool isInstallGlobalCommand() =>
parsedArgs?.command?.name == 'sidekick' &&
parsedArgs!.arguments.contains('install-global');

if (!enableAutoInstall &&
!isRunningTabCompletionCommand() &&
!_isRunningTabCompletionCommand(parsedArgs) &&
!isInstallGlobalCommand()) {
final command =
yellow('./${SidekickContext.cliName} sidekick install-global');
Expand All @@ -320,24 +322,16 @@ class SidekickCommandRunner<T> extends CompletionCommandRunner<T> {
}

try {
if (_isUpdateCheckEnabled &&
!_isSidekickCliUpdateCommand(parsedArgs) &&
!isRunningTabCompletionCommand()) {
// print warning if the user didn't fully update their CLI
_checkCliVersionIntegrity();
// print warning if CLI update is available
// TODO start the update check in the background at command start
// TODO prevent multiple update checks when a command start another command
await _checkForUpdates();
}
(await updateCheck)?.call();
} finally {
unmount();
}
}
}

/// Print a warning if the CLI isn't up to date
Future<void> _checkForUpdates() async {
Future<void Function()> _checkForUpdates() async {
void Function()? updateCheck;
try {
final updateFuture = VersionChecker.isDependencyUpToDate(
Copy link
Contributor

@passsy passsy Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The try catch is now useless, because the updateFuture is not awaited.
The error of VersionChecker.isDependencyUpToDate() is never caught and would crash the CLI

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Future itself is awaited in Line 345 in combination with the timeout
Errors are caught and result in returning null

package: SidekickContext.sidekickPackage,
Expand All @@ -346,15 +340,21 @@ class SidekickCommandRunner<T> extends CompletionCommandRunner<T> {
);
// If it takes too long, don't wait for it
final isUpToDate = await updateFuture.timeout(const Duration(seconds: 3));

if (!isUpToDate) {
printerr(
'${yellow('Update available!')}\n'
'Run ${cyan('${SidekickContext.cliName} sidekick update')} to update your CLI.',
);
updateCheck = () => printerr(
'${yellow('Update available!')}\n'
'Run ${cyan('${SidekickContext.cliName} sidekick update')} to update your CLI.',
);
}
} catch (_) {
/* ignore */
}
return () {
// print warning if the user didn't fully update their CLI
_checkCliVersionIntegrity();
updateCheck?.call();
};
}

/// Print a warning if the user manually updated the sidekick_core
Expand Down Expand Up @@ -397,6 +397,18 @@ class SidekickCommandRunner<T> extends CompletionCommandRunner<T> {
}
}

/// Returns true if the executed command is a tab completion command
///
/// don't print anything additionally when running the hidden tab completion command (runs in the background when pressing tab),
/// otherwise anything that is printed will also be used as suggestion
bool _isRunningTabCompletionCommand(ArgResults? parsedArgs) {
final reservedCommands = [
HandleCompletionRequestCommand.commandName,
InstallCompletionFilesCommand.commandName,
];
return reservedCommands.contains(parsedArgs?.command?.name);
}

/// Returns true if the command executed from [parsedArgs] is [UpdateCommand]
///
/// Copied and adapted from CommandRunner.runCommand
Expand Down