Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add alert, confirm, and prompt #7507

Merged
merged 6 commits into from
Oct 13, 2020
Merged

feat: add alert, confirm, and prompt #7507

merged 6 commits into from
Oct 13, 2020

Conversation

kt3k
Copy link
Member

@kt3k kt3k commented Sep 16, 2020

This PR adds alert, confirm, and prompt functions from web standards.

You can write simple interaction script like the below with this change.

const name = prompt('What is your name?');
console.log(`Hello ${name}!`);
$ ./target/debug/deno run test.ts
What is your name? John
Hello John!

Maybe these features are not so important for advanced users, but I think these could be very helpful for beginners of Deno or learners of the language.

closes #4257

nayeemrmn
nayeemrmn requested changes Sep 16, 2020
View reviewed changes
Copy link
Collaborator

@nayeemrmn nayeemrmn left a comment

Choose a reason for hiding this comment

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

When not interactive, we should do what browsers do when dialog boxes are disabled.

cli/rt/41_prompt.js Outdated Show resolved Hide resolved
cli/rt/41_prompt.js Outdated Show resolved Hide resolved
cli/rt/41_prompt.js Outdated Show resolved Hide resolved
cli/dts/lib.deno.shared_globals.d.ts Outdated Show resolved Hide resolved
nayeemrmn
nayeemrmn reviewed Sep 16, 2020
View reviewed changes
cli/rt/41_prompt.js Outdated Show resolved Hide resolved
nayeemrmn
nayeemrmn reviewed Sep 16, 2020
View reviewed changes
cli/rt/41_prompt.js Outdated Show resolved Hide resolved
nayeemrmn
nayeemrmn reviewed Sep 16, 2020
View reviewed changes
cli/rt/41_prompt.js Outdated Show resolved Hide resolved
nayeemrmn
nayeemrmn approved these changes Sep 16, 2020
View reviewed changes
Copy link
Collaborator

@nayeemrmn nayeemrmn left a comment

Choose a reason for hiding this comment

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

LGTM

@kryptish
Copy link

I like this, anything speaking against merging this? =)

@ry ry added this to the 1.5.0 milestone Sep 18, 2020
@bartlomieju
Copy link
Member

@kt3k this looks great! I love how simple the implementation is and having this API enables users to create simple CLI scripts without additional libraries. Please the branch so we can land it

@kt3k
Copy link
Member Author

kt3k commented Oct 12, 2020 •
edited
Loading

@bartlomieju Thank you for the feedbacks! Rebased the branch! I'll add the handling (and tests) of yes, Yes, etc in confirm function a little later. Keep it as is for now.

bartlomieju
bartlomieju approved these changes Oct 13, 2020
View reviewed changes
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, great feature @kt3k!

@bartlomieju bartlomieju merged commit 0dcaea7 into denoland:master Oct 13, 2020
@bartlomieju
Copy link
Member

Yikes, it looks like @kitsonk had a lot of concerns for this API in #4257. @kitsonk are you still against them? If so, I will do the revert.

bartlomieju added a commit to bartlomieju/deno that referenced this pull request Oct 13, 2020
@bartlomieju
Revert "feat: add alert, confirm, and prompt (denoland#7507)"
7fc296d 
This reverts commit 0dcaea7.
@bartlomieju bartlomieju mentioned this pull request Oct 13, 2020
Closed
@David-Else
Copy link

These are awesome. I hope to teach kids programming and see Deno as a perfect way to provide everything they will need in one executable. I can even teach typed vs untyped going from JS to TS, these kinds of simple API's to do essential things are so important. It is the difference between friendly and easy to understand vs annoying and pointless time-wasting.

@kt3k kt3k deleted the feature/prompt branch October 30, 2020 05:35
@Bestulo
Copy link

Bestulo commented Dec 30, 2020 •
edited
Loading

Is there any way to use these as promise instead of blocking?

edit: guess it could be a child process that reads from console but that sounds over the top

@Soremwar
Copy link
Contributor

@Bestulo Alert, confirm and prompt are all abstractions over a simple stdin/stdout program, it's quite easy to implement asynchronously without this API

@kt3k kt3k mentioned this pull request Apr 14, 2023
Merged
@solb solb mentioned this pull request Mar 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikknaake erikknaake erikknaake left review comments

@crypt096 crypt096 crypt096 approved these changes

@nayeemrmn nayeemrmn nayeemrmn approved these changes

@bartlomieju bartlomieju bartlomieju approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.

Request: alert, confirm, prompt, and their async counterpart
10 participants
@kt3k @kryptish @bartlomieju @David-Else @Bestulo @Soremwar @nayeemrmn @crypt096 @erikknaake @ry