Skip to content

Conversation

@jsparkdev
Copy link
Contributor

@jsparkdev jsparkdev commented Apr 1, 2025

Fix a bug where the placeholder value is not used when no value is provided to the text function.

before:

image

After:
image

@changeset-bot
Copy link

changeset-bot bot commented Apr 1, 2025

🦋 Changeset detected

Latest commit: bc1c865

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@clack/core Patch
@clack/prompts Patch

Not sure what this means? Click here to learn what changesets are.

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

@43081j
Copy link
Collaborator

43081j commented Apr 1, 2025

im not sure this is right 🤔

the placeholder is something to render but isn't the actual value, similar to how an <input> works. an <input> renders the placeholder, but its value is empty unless you type into it.

edit: ignore me on this bit. the placeholder is basically a default value you can <tab> into

do you have an example of how you're using it? i.e. how you produced whats in the screenshot

@jsparkdev
Copy link
Contributor Author

jsparkdev commented Apr 1, 2025

@43081j Thanks for your review!

The following is an example that uses the code from https://bomb.sh/docs/basics/getting-started/#quick-start directly.

2025-04-02.8.29.22.mov

Here is how the Vite CLI works.

2025-04-02.8.33.14.mov

The Astro CLI is:

2025-04-02.8.39.08.mov

Actually, it looks like Vite CLI uses the prompts package, but its behavior is different, haha.

@43081j
Copy link
Collaborator

43081j commented Apr 2, 2025

ok i understand now 👍

i think we should fix this in the core rather than specifically in the text prompt

basically, if you press <tab> instead of enter right away, it should fill the value in from the placeholder

the problem seems to be that if you don't tab it in, and just press enter, it doesn't actually set the value.

so we should probably update the core prompt code to set value on submit. i.e. when submitted, if value === undefined && placeholder !== undefined, set the value = placeholder or some such thing

@jsparkdev
Copy link
Contributor Author

jsparkdev commented Apr 2, 2025

@43081j Thanks for great review.

I've updated the core to behave as follows when there is no input value:

First and foremost, set the default value as the value.
If the default value is not set, set the placeholder as the value.
If both the default value and the placeholder are absent, set an empty string as the value.

Copy link
Member

@dreyfus92 dreyfus92 left a comment

Choose a reason for hiding this comment

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

first of all thank you @jsparkdev, for taking the time and taking a look at this. this solution might work in some cases, but i think it might have some limitations that could lead into more issues, like ambiguity with falsy values e.g. if opts.defaultValue is an empty string (''), it skips to opts.placeholder, which might not be the desired behavior if an empty string is a valid default value. also we might face type safety issues, this assumes that opts.defaulValue and opts.placeholder are either strings or falsy, if the could be other types, the result might be unexpected or inconsistent. what about if we try something else? i haven't tested this myself. cc @43081j

@43081j
Copy link
Collaborator

43081j commented Apr 2, 2025

i don't think it belongs in the text prompt

in Prompt, we do this:

if (char === '\t' && this.opts.placeholder) {
if (!this.value) {
this.rl?.write(this.opts.placeholder);
this.emit('value', this.opts.placeholder);
}
}

i feel like we should just check here for 'return' too. i.e. if ((char === '\t' || key?.name === 'return') && this.opts.placeholder)

then later, we handle submission etc because return was pressed

@jsparkdev jsparkdev changed the title fix: update text function to fixing bug fix: use placeholder as value when input is empty Apr 7, 2025
@pkg-pr-new
Copy link

pkg-pr-new bot commented Apr 7, 2025

@example/basic@example/changesets

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

commit: bc1c865

@natemoo-re natemoo-re added the backport This PR should be cherry-picked onto v0 label Apr 8, 2025
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Great to see you here, thanks so much for the PR! 🎉

@natemoo-re natemoo-re merged commit a4f5034 into bombshell-dev:main Apr 8, 2025
5 checks passed
@jsparkdev jsparkdev deleted the placeholder branch April 8, 2025 02:47
natemoo-re added a commit that referenced this pull request Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport This PR should be cherry-picked onto v0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants