Skip to content

Commit 145c39f

Browse files
committed
fix: address the first review round on the command API and shortcuts
- A legacy command's `skipOptionsValidation` still counts, as the deprecated spelling of `allowUnknownOptions`. - Option priming puts the parser back when validation throws, and an error is reported once however many dispatch levels rethrow it. - `open` passes IDE paths as arguments instead of shell strings, resets the visionOS platform override in a `finally`, and logs a failed launch. - `preview` no longer falls through from the bun case to npm. - The prepare controller removes its bundler listener even while the watcher is paused; a restart re-reads the session's device descriptors so a device stopped meanwhile is left alone; `waitForExit` answers at once for a child that has already exited. - The `c` shortcut spawns the CLI through `process.execPath` and reports a spawn failure instead of crashing; shortcuts attach over IPC only when an IPC channel exists; a visionOS child receives forwarded keys and is stopped by `c`. - The guide no longer names the removed in-process aliases and describes the argument specs a definition accepts.
1 parent ffa6afa commit 145c39f

17 files changed

Lines changed: 225 additions & 42 deletions

‎defining-commands.md‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,9 @@ Validation happens where you can see it
5757

5858
A definition is checked at the moment `defineCommand` is called, not when the
5959
command eventually runs. A misspelled field, a missing `run`, an option
60-
declared with something other than the five helpers, an `arguments` value
61-
outside `"none" | "any"` — each throws immediately, naming the command and the
62-
accepted form. The class form's meta is checked the same way at the
60+
declared with something other than the five helpers, an `arguments` value that
61+
is neither `"none"`, `"any"` nor a list of argument specs — each throws
62+
immediately, naming the command and the accepted form. The class form's meta is checked the same way at the
6363
`Command({ ... })` call, which also rejects handlers passed there; only a
6464
missing `run` method waits until the definition is first read:
6565

@@ -1047,8 +1047,6 @@ argument list to a child that declares fewer is a rejection, not a wider check.
10471047

10481048
`canExecuteCommand` follows `runCommand` in everything else: the same option
10491049
priming and restoration, the same routing of a parent name to its subcommand.
1050-
The deprecated `canExecuteCommandInProcess` and `executeCommandInProcess`
1051-
call the two methods with a name.
10521050

10531051
### Key shortcuts
10541052

‎lib/commands/open.ts‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -89,15 +89,14 @@ async function openAndroidStudioProject(
8989

9090
const os = currentPlatform();
9191
if (os === "darwin") {
92-
$childProcess.exec(`open -a "${studioPath}" ${androidDir}`);
93-
} else if (os === "win32") {
92+
$childProcess.execFile("open", ["-a", studioPath, androidDir]);
93+
} else if (os === "win32" || os === "linux") {
9494
const child = $childProcess.spawn(studioPath, [androidDir], {
9595
detached: true,
9696
stdio: "ignore",
9797
});
98+
child.on("error", (error: Error) => $logger.error(error.message));
9899
child.unref();
99-
} else if (os === "linux") {
100-
$childProcess.exec(`${studioPath} ${androidDir}`);
101100
}
102101
}
103102

@@ -142,7 +141,7 @@ async function openXcodeProject(
142141
if (fs.existsSync(xcprojectFile)) {
143142
$xcodeSelectService
144143
.getDeveloperDirectoryPath()
145-
.then(() => $childProcess.exec(`open ${xcprojectFile}`, {}))
144+
.then(() => $childProcess.execFile("open", [xcprojectFile]))
146145
.catch((e) => {
147146
$logger.error(e.message);
148147
});
@@ -157,8 +156,11 @@ async function openVisionOSProject(
157156
isInteractive: boolean,
158157
): Promise<void> {
159158
$options.platformOverride = "visionOS";
160-
await openXcodeProject(context, "visionos", isInteractive);
161-
$options.platformOverride = null;
159+
try {
160+
await openXcodeProject(context, "visionos", isInteractive);
161+
} finally {
162+
$options.platformOverride = null;
163+
}
162164
}
163165

164166
const openCommandOptions = {

‎lib/commands/preview.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ export class PreviewCommand extends Command({
8181
break;
8282
case PackageManagers.bun:
8383
installCommand = "bun add --dev @nativescript/preview-cli";
84+
break;
8485
case PackageManagers.npm:
8586
default:
8687
installCommand = "npm install --save-dev @nativescript/preview-cli";

‎lib/common/definitions/commands.d.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ interface ICommand extends ICommandOptions {
2525
* own declared options are still merged and checked.
2626
*/
2727
allowUnknownOptions?: boolean;
28+
/** @deprecated Use allowUnknownOptions. */
29+
skipOptionsValidation?: boolean;
2830

2931
/**
3032
* Describes the action that will be executed after the command succeeds.

‎lib/common/errors.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,10 @@ import { ErrorCodes } from "./enums";
99
import { IInjector } from "./definitions/yok";
1010
import { injector } from "./yok";
1111

12+
const COMMAND_ERROR_REPORTED = Symbol.for(
13+
"nativescript:cli:commandErrorReported",
14+
);
15+
1216
// we need this to overwrite .stack property (read-only in Error)
1317
function Exception() {
1418
/* intentionally left blank */
@@ -217,6 +221,16 @@ export class Errors implements IErrors {
217221
error: any,
218222
printCommandHelpSuggestion: () => Promise<void>,
219223
): Promise<void> {
224+
// A nested in-process dispatch reports and rethrows, and so does each
225+
// level above it up to the command line, so the same error reaches here
226+
// once per level.
227+
if (error && typeof error === "object") {
228+
if (error[COMMAND_ERROR_REPORTED]) {
229+
return;
230+
}
231+
error[COMMAND_ERROR_REPORTED] = true;
232+
}
233+
220234
const logger = this.$injector.resolve("logger");
221235
const loggerLevel: string = logger.getLevel().toUpperCase();
222236
const printCallStack =

‎lib/common/services/commands-service.ts‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,9 @@ interface IInProcessDispatch {
3232
commandName: string;
3333
}
3434

35+
const allowsUnknownOptions = (command: ICommand): boolean =>
36+
command.allowUnknownOptions ?? command.skipOptionsValidation ?? false;
37+
3538
class CommandArgumentsValidationHelper {
3639
constructor(
3740
public isValid: boolean,
@@ -259,7 +262,7 @@ export class CommandsService
259262
const dashedOptions = command ? command.dashedOptions : null;
260263
this.$options.validateOptions(
261264
dashedOptions,
262-
command && command.allowUnknownOptions,
265+
command && allowsUnknownOptions(command),
263266
);
264267
}
265268

@@ -547,16 +550,24 @@ export class CommandsService
547550
private primeOptions(command: ICommand): () => void {
548551
const declaredOptions = { ...this.$options.options };
549552
const parsedArgv = this.$options.argv;
550-
551-
this.$options.validateOptions(
552-
command.dashedOptions,
553-
command.allowUnknownOptions,
554-
);
555-
556-
return () => {
553+
const restore = (): void => {
557554
this.$options.options = declaredOptions;
558555
this.$options.argv = parsedArgv;
559556
};
557+
558+
// validateOptions merges the command's declarations into the live table
559+
// before it can throw, so a failed priming has to be undone here.
560+
try {
561+
this.$options.validateOptions(
562+
command.dashedOptions,
563+
allowsUnknownOptions(command),
564+
);
565+
} catch (error) {
566+
restore();
567+
throw error;
568+
}
569+
570+
return restore;
560571
}
561572

562573
private async canExecuteResolvedCommand(

‎lib/common/test/unit-tests/errors.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,27 @@ describe("errors", () => {
3232
return testInjector;
3333
};
3434

35+
describe("reportCommandError", () => {
36+
it("reports one error once, whichever dispatch level asks", async () => {
37+
const testInjector = getTestInjector();
38+
const errors: IErrors = testInjector.resolve("errors");
39+
const logger: CommonLoggerStub = testInjector.resolve("logger");
40+
const error = Object.assign(new Error("Unable to open the project."), {
41+
suggestCommandHelp: true,
42+
});
43+
let suggestions = 0;
44+
const suggest = async (): Promise<void> => {
45+
suggestions++;
46+
};
47+
48+
await errors.reportCommandError(error, suggest);
49+
await errors.reportCommandError(error, suggest);
50+
51+
assert.equal(logger.errorOutput, "Unable to open the project.\n");
52+
assert.equal(suggestions, 1);
53+
});
54+
});
55+
3556
describe("beginCommand", () => {
3657
let testInjector: IInjector;
3758
let errors: IErrors;

‎lib/controllers/prepare-controller.ts‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -126,16 +126,19 @@ export class PrepareController
126126
this.watchersData[projectDir][platformLowerCase] &&
127127
this.watchersData[projectDir][platformLowerCase].hasWebpackCompilerProcess
128128
) {
129-
const watcherData = this.watchersData[projectDir][platformLowerCase];
130129
await this.$bundlerCompilerService.stopBundlerCompiler(platformLowerCase);
131-
if (watcherData.bundlerCompilerHandler) {
132-
this.$bundlerCompilerService.removeListener(
133-
BUNDLER_COMPILATION_COMPLETE,
134-
watcherData.bundlerCompilerHandler,
135-
);
136-
watcherData.bundlerCompilerHandler = null;
137-
}
138-
watcherData.hasWebpackCompilerProcess = false;
130+
this.watchersData[projectDir][
131+
platformLowerCase
132+
].hasWebpackCompilerProcess = false;
133+
}
134+
135+
const watcherData = this.watchersData?.[projectDir]?.[platformLowerCase];
136+
if (watcherData?.bundlerCompilerHandler) {
137+
this.$bundlerCompilerService.removeListener(
138+
BUNDLER_COMPILATION_COMPLETE,
139+
watcherData.bundlerCompilerHandler,
140+
);
141+
watcherData.bundlerCompilerHandler = null;
139142
}
140143
}
141144

‎lib/controllers/run-controller.ts‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -272,12 +272,21 @@ export class RunController extends EventEmitter implements IRunController {
272272
const useHotModuleReload =
273273
!!liveSyncProcessInfo.liveSyncInfo?.useHotModuleReload;
274274

275-
const deviceAction = async (device: Mobile.IDevice) => {
276-
const deviceDescriptor = _.find(
277-
deviceDescriptors,
278-
(dd) => dd.identifier === device.deviceInfo.identifier,
275+
// A device stopped while this action waited in the chain must not be restarted.
276+
const findCurrentDescriptor = (device: Mobile.IDevice) =>
277+
_.find(
278+
this.$liveSyncProcessDataService.getDeviceDescriptors(projectDir),
279+
(dd) =>
280+
dd.identifier === device.deviceInfo.identifier &&
281+
_.some(deviceDescriptors, (d) => d.identifier === dd.identifier),
279282
);
280283

284+
const deviceAction = async (device: Mobile.IDevice) => {
285+
const deviceDescriptor = findCurrentDescriptor(device);
286+
if (!deviceDescriptor) {
287+
return;
288+
}
289+
281290
try {
282291
const platformLiveSyncService =
283292
this.$liveSyncServiceResolver.resolveLiveSyncService(
@@ -330,12 +339,9 @@ export class RunController extends EventEmitter implements IRunController {
330339
};
331340

332341
await this.addActionToChain(projectDir, () =>
333-
this.$devicesService.execute(deviceAction, (device: Mobile.IDevice) =>
334-
_.some(
335-
deviceDescriptors,
336-
(deviceDescriptor) =>
337-
deviceDescriptor.identifier === device.deviceInfo.identifier,
338-
),
342+
this.$devicesService.execute(
343+
deviceAction,
344+
(device: Mobile.IDevice) => !!findCurrentDescriptor(device),
339345
),
340346
);
341347
}

‎lib/services/bundler/bundler-compiler-service.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1080,6 +1080,10 @@ export class BundlerCompilerService
10801080
childProcess: child_process.ChildProcess,
10811081
timeoutMs: number,
10821082
): Promise<boolean> {
1083+
if (childProcess.exitCode !== null || childProcess.signalCode !== null) {
1084+
return Promise.resolve(true);
1085+
}
1086+
10831087
return new Promise<boolean>((resolve) => {
10841088
const settle = (exited: boolean) => {
10851089
clearTimeout(timer);

0 commit comments

Comments
 (0)