fix(functions): honor non-string defaults in params select prompts - #11059
Ishkirat-Singh wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where boolean and integer defaults in select prompts for function parameters were not honored, resulting in the first option being preselected. This is resolved by stringifying the resolvedDefault value in promptSelect so it matches the stringified choice values. Additionally, unit tests have been added to verify this behavior for both boolean and integer defaults, and the changelog has been updated. I have no feedback to provide.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11059 +/- ##
=======================================
Coverage ? 60.91%
=======================================
Files ? 651
Lines ? 43805
Branches ? 9011
=======================================
Hits ? 26685
Misses ? 14878
Partials ? 2242 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
IzaakGough
left a comment
There was a problem hiding this comment.
Lgtm! Nice fix. Just needs a conflict in the changelog resolving
def40a4 to
10239c9
Compare
|
Thanks! Rebased onto main and resolved the CHANGELOG conflict (the release reset the file), and updated the tests for the new resolveParams options object. Should be good to merge. |
promptSelect stringifies every choice value but passed the resolved default through unchanged, so a boolean or integer default never matched a choice and inquirer fell back to the first option. Stringify the default the same way. Fixes firebase#11053
10239c9 to
008ee62
Compare
|
@IzaakGough rebased onto main again (it had fallen behind by two commits), no conflicts this time. Ready to merge whenever you get a moment. |
|
@Ishkirat-Singh - I just merged a significant change to parameter prompting for functions (#11086). It'd be good to update to latest and make sure this and #11062 play nicely with that change. But these seem like good fixes to land. Thank you for the contributions. |
# Conflicts: # CHANGELOG.md
|
Thanks @ajperel — updated to latest, and I checked both against #11086. Both fixes are still needed after your rewrite, and both still apply cleanly:
|
ajperel
left a comment
There was a problem hiding this comment.
I agree this is a good change and just need to fix CHANGELOG
| default: resolvedDefault as string, | ||
| // Choice values are stringified below, so the default must be too or a | ||
| // boolean/number default never matches and the first option is preselected. | ||
| default: resolvedDefault?.toString(), |
There was a problem hiding this comment.
I think at this point we could make a larger, better change where we don't stringify the values or defaults and get rid of the converters. But I'm not 100% confident without trying it and this smaller change does work, fixes a bug, and I appreciate the contribution.
Description
promptSelectstringifies every choice value (option.value.toString()) but passed the resolved default to theselectprompt unchanged. A boolean or integer default therefore never matched a choice, and inquirer fell back to preselecting the first option. In non-interactive mode the same raw default was handed straight to the converter, whereisTruthyInputcalls.toLowerCase()on it.This stringifies the default the same way the choices are, so
defineBoolean/defineIntparams with aselectinput preselect their declared default.promptSelectMultiplewas checked as well: its values are already strings (T extends string), so it is not affected by this particular mismatch.Fixes #11053
Scenarios Tested
src/deploy/functions/params.spec.tscovering a boolean select default (false) and an int select default (2), asserting thedefaultpassed to the prompt and the resolvedParamValue.npx mocha src/deploy/functions/params.spec.ts: 20 passing. Both new tests fail againstmainwithout the fix.Sample Commands
N/A — the change is in the interactive
firebase deploy --only functionsparam prompt.