feat(spinner): add clear param to stop function

[hyperz111]

Fixes #401

[changeset-bot]

⚠️ No Changeset found

Latest commit: 5aae76b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[pkg-pr-new]

@example/basic • @example/changesets

npm i https://pkg.pr.new/bombshell-dev/clack/@clack/core@402

npm i https://pkg.pr.new/bombshell-dev/clack/@clack/prompts@402

commit: 5aae76b

[43081j]

my concern here is the signature doesn't align anymore:

const stop = (msg = '', code = 0, clear = false): void

msg is basically ignored if clear === true. i.e. stop("foo bar", 0, true) wouldn't use "foo bar" at all

i wonder if we should have a clear method instead, for this reason. and it would be implied clearing the spinner also means stopping it

thoughts @dreyfus92 ?

[hyperz111]

my concern here is the signature doesn't align anymore:

const stop = (msg = '', code = 0, clear = false): void

msg is basically ignored if clear === true. i.e. stop("foo bar", 0, true) wouldn't use "foo bar" at all

i wonder if we should have a clear method instead, for this reason. and it would be implied clearing the spinner also means stopping it

thoughts @dreyfus92 ?

If we follow the #401 suggestion, this will break progress component.

[dreyfus92]

hey peeps, i agree with @43081j. it would be best to create a clear() method which doesn't create confusion about params behavior, and it is also backward compatible (doesn't change existing stop() signature).

export interface SpinnerResult {
    start(msg?: string): void;
    stop(msg?: string, code?: number): void;
    clear(code?: boolean): void; // NEW: silently stops spinner
    message(msg?: string): void;
    readonly isCancelled: boolean;
}

[43081j]

this will break progress component

can you explain why it would break the progress prompt?

could we not just add clear there too?

[hyperz111]

can you explain why it would break the progress prompt?

If we change the stop method.

BEFORE:

const stop = (msg = '', code = 0): void => {
	if (!isSpinnerActive) return;
	isSpinnerActive = false;
	clearInterval(loop);
	clearPrevMessage();
	const step =
		code === 0
			? color.green(S_STEP_SUBMIT)
			: code === 1
				? color.red(S_STEP_CANCEL)
				: color.red(S_STEP_ERROR);
	_message = msg ?? _message;
	if (indicator === 'timer') {
		output.write(`${step}  ${_message} ${formatTimer(_origin)}\n`);
	} else {
		output.write(`${step}  ${_message}\n`);
	}
	clearHooks();
	unblock();
};

AFTER:

- const stop = (msg = '', code = 0): void => {
+ const stop = (msg, code = 0): void => {
	if (!isSpinnerActive) return;
	isSpinnerActive = false;
	clearInterval(loop);
	clearPrevMessage();
	const step =
		code === 0
			? color.green(S_STEP_SUBMIT)
			: code === 1
				? color.red(S_STEP_CANCEL)
				: color.red(S_STEP_ERROR);
-	_message = msg ?? _message;
-    if (indicator === 'timer') {
-		output.write(`${step}  ${_message} ${formatTimer(_origin)}\n`);
-	} else {
-		output.write(`${step}  ${_message}\n`);
+	if (msg != null) {
+		_message = msg;
+		if (indicator === 'timer') {
+			output.write(`${step}  ${_message} ${formatTimer(_origin)}\n`);
+		} else {
+			output.write(`${step}  ${_message}\n`);
+		}
	}
	clearHooks();
	unblock();
};

When we run the test (maybe we must update the test):

  Snapshots  28 failed
 Test Files  2 failed | 12 passed (14)
      Tests  28 failed | 356 passed (384)
   Start at  13:18:24
   Duration  15.13s (transform 5.40s, setup 0ms, collect 29.24s, tests 10.29s, environment 48ms, prepare 19.68s)

could we not just add clear there too?

In some spinner libraries, clear method is JUST for cleaning the spinner and NOT stopping the spinner.
If we don't want to follow some libraries convention, maybe we can add clear method instead.

[43081j]

@delucis has also opened #405

i wonder if we should try land that first, then add a clear method too

the one other popular spinner (ignoring ora since it is going away) - nanospinner, doesn't have a clear method. none of the ones i know have this capability.

so i'd be happy to have a clear method for this