nonEmpty param input option support by Berlioz · Pull Request #10678 · firebase/firebase-tools

Berlioz · 2026-06-17T22:49:02Z

Also gets rid of that immutable copy-paste mistake which has never done anything.

gemini-code-assist

Code Review

This pull request introduces a 'nonEmpty' option to parameter definitions to reject empty strings and empty arrays during CLI prompting, propagating this configuration through various prompt helper functions. The review feedback highlights a critical TypeScript compilation error caused by changing the signature of 'promptResourceString' (which breaks an existing caller), and suggests correcting an inaccurate error message in 'promptSelectMultiple' when no options are selected.

inlined

I think I recommend that nonEmpty is a part of input? we can talk offline.

inlined · 2026-06-18T20:55:14Z

      prompt += ` \n(${param.description})`;
    }
-    return promptText<boolean>(prompt, param.input, resolvedDefault, isTruthyInput);
+    return promptText<boolean>(prompt, param.input, resolvedDefault, false, isTruthyInput);


please always leave a comment on any boolean literal ever used as a parameter

inlined · 2026-06-18T20:56:05Z

  prompt: string,
  input: ResourceInput,
  projectId: string,
+  disallowEmpty: boolean | undefined,


why disallowEmpty and nonEmpty? Also, why mandatory but supports undefined?

Yeah (naming aside) this could just be disallowEmpty: boolean = false so there's a default value for the case where we have undefined from it not being in functions.yaml right? (same elsewhere).

I have a hunch on why the name change from nonEmpty to disallowEmpty but I'd like to hear Victor's answer :) ... I do think it's a debateable shift.

I think changing the var name from nonEmpty to disallowEmpty as it moves from the wire format into the function calling chain makes sense: nonEmpty is describing something about the input itself, while disallowEmpty is more a modifier on the behavior of the prompt: "this prompt is nonempty" seems more vague than "this prompt disallows empty".

That's very much a subjective take though and if you two feel like it's an unnecessary or bad naming decision I really don't mind reverting it.

The boolean | undefined thing was just me being dumb and forgetting that typescript had optional parameters for a second. Fixed.

I get the move from describing the input to enforcing the constraint (that was what I thought you were doing). I'd say if we want to make that distinction we could have a slightly better throughline with a different name.

nonEmpty -> enforceNonEmpty or something like that.

oh, yeah, that's a good compromise

inlined · 2026-06-18T21:03:52Z

  input?: ParamInput<T>;
+
+  // Reject the empty string and empty array as valid input.
+  nonEmpty?: boolean;


I wonder how hard it is to make the field only present for the right types. At minimum you can say:

type TypeIfExtends<Result, Test, Base> = Test extends Base ? Result : never; // ... nonEmpty: TypeIfExtends<boolean, T, string | string[]>

I am not as confident about reading type system stuff as you are but my instinct is that this is a huge loss of readability to double-enforce a constraint that the SDK can already enforce much more clearly.

inlined · 2026-06-18T21:04:40Z

      prompt,
      param.input,
      resolvedDefault,
+      param.nonEmpty,


why is nonEmpty not part of input?

If it is (which from what I understand with a bit of research I would support) do we also need to update the API proposal to be clear about that?

Or we enforce nonEmpty at deployment time and not just input prompting time?

I guess a difference between functions and extensions is users can edit params after the input form by hand editing .env files (or could add nonEmpty after a first deployment that writes an empty string).

Update: we talked it out, updated the SDK, and yeah, moved to input fields.

ajperel

Chiming in with a few thoughts since I went through this PR trying to understand it. But overall, I think just having Thomas's LG (and not blocking on more review from me) is fine.

ajperel · 2026-06-25T00:07:06Z

  prompt: string,
  input: ResourceInput,
  projectId: string,
+  disallowEmpty: boolean | undefined,


Yeah (naming aside) this could just be disallowEmpty: boolean = false so there's a default value for the case where we have undefined from it not being in functions.yaml right? (same elsewhere).

I have a hunch on why the name change from nonEmpty to disallowEmpty but I'd like to hear Victor's answer :) ... I do think it's a debateable shift.

ajperel · 2026-06-25T00:09:03Z

      prompt,
      param.input,
      resolvedDefault,
+      param.nonEmpty,


If it is (which from what I understand with a bit of research I would support) do we also need to update the API proposal to be clear about that?

Or we enforce nonEmpty at deployment time and not just input prompting time?

I guess a difference between functions and extensions is users can edit params after the input form by hand editing .env files (or could add nonEmpty after a first deployment that writes an empty string).

inlined · 2026-07-10T01:08:51Z

+      prompt,
+      param.input,
+      resolvedDefault,
+      false, // enforceNonEmpty


Doesn't text input support nonempty?

Sure does. Straight up copy-paste error.

inlined · 2026-07-10T01:09:50Z

+      return promptSelectMultiple<string>(
+        prompt,
+        forgedInput,
+        undefined,


please comment on what parameter a literal is

inlined · 2026-07-10T01:10:04Z

        `Warning: unknown resource type ${input.resource.type}; defaulting to raw text input...`,
      );
-      return promptText<string[]>(prompt, { text: {} }, undefined, (res: string) => res.split(","));
+      return promptText<string[]>(prompt, { text: {} }, undefined, enforceNonEmpty, (res: string) =>


same. Comment on what undefined is (I know this is preexisting)

inlined · 2026-07-10T01:10:37Z

    }
  }
+  if (enforceNonEmpty && res === "") {
+    logger.error(`Input cannot be the empty string, retrying...`);


Can we be a little less nerdy? E.g. "A value is required, retrying..."

supports the nonEmpty param option for string and []string type params

9581b8e

Berlioz requested review from ajperel and inlined June 17, 2026 22:49

gemini-code-assist Bot reviewed Jun 17, 2026

View reviewed changes

Comment thread src/deploy/functions/params.ts

Comment thread src/deploy/functions/params.ts Outdated

Berlioz added 2 commits June 17, 2026 15:50

forgot to hit save file

05d13e9

clean up error string

5241c1f

inlined reviewed Jun 18, 2026

View reviewed changes

ajperel reviewed Jun 25, 2026

View reviewed changes

Berlioz added 3 commits July 6, 2026 14:47

changed wire format

525dc53

Merge remote-tracking branch 'origin' into vsfan_nonempty_params

9d3143a

lint

53aa388

Berlioz changed the title ~~WIP: nonEmpty param option support~~ nonEmpty param input option support Jul 6, 2026

Berlioz added 2 commits July 7, 2026 16:31

style

c98beec

Merge remote-tracking branch 'origin' into vsfan_nonempty_params

6a8a5ad

Berlioz assigned inlined Jul 10, 2026

inlined reviewed Jul 10, 2026

View reviewed changes

Berlioz added 3 commits July 10, 2026 01:52

review

d9d1fa1

Merge branch 'main' into vsfan_nonempty_params

3c2c075

Merge branch 'main' into vsfan_nonempty_params

9d8fe6e

inlined approved these changes Jul 10, 2026

View reviewed changes

changelog

e53b5f9

Berlioz enabled auto-merge (squash) July 10, 2026 20:36

Berlioz disabled auto-merge July 10, 2026 20:43

Berlioz merged commit a68434e into main Jul 10, 2026
53 checks passed

Berlioz deleted the vsfan_nonempty_params branch July 10, 2026 20:53

Uh oh!

Conversation

Berlioz commented Jun 17, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

inlined left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajperel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants