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.

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

fix: match Deno version of prompt #160

Merged
merged 3 commits into from
Dec 18, 2023
Merged

fix: match Deno version of prompt #160

merged 3 commits into from
Dec 18, 2023

Conversation

uncenter
Copy link
Contributor

@uncenter uncenter commented Nov 26, 2023

Removes an extra space in the prompt, adds the default message, and fixes the logic to match the Deno implementation.
I compared it to https://github.com/denoland/deno/blob/main/runtime/js/41_prompt.js#L33-L50, correct me if this is the wrong place please!

The main part of the Deno implementation looks like this:
https://github.com/denoland/deno/blob/a4ec7dfae01485290af91c62c1ce17a742dcb104/runtime/js/41_prompt.js#L34-L44

  defaultValue ??= null;


  if (!isatty(stdin.rid)) {
    return null;
  }


  if (defaultValue) {
    message += ` [${defaultValue}]`;
  }


  message += " ";

defaultValue is assigned to null if defaultValue is undefined or null (see nullish coalescing assignment (??=)). Then, if defaultValue is truthy, [${defaultValue}] is added to message.

I'll admit I'm quite confused why the defaultValue ??= null; line is needed; both null and undefined are not truthy soo the if (defaultValue) { part works the same either way...

Anyway, the logic for this shim implementation used to look like this:
https://github.com/denoland/node_shims/blob/main/packages/shim-prompts/src/index.ts#L35

defaultValue == null ? "" : `[${defaultValue}]`

Which, if expanded to an if statement and rearranged, would look like this:

if (defaultValue != null) {
  message +=  `[${defaultValue}]`;
}

Because the defaultValue ??= null; line is not needed (see above), the difference is defaultValue != null in the shim vs defaultValue in the Deno version. Hence, I've changed it to be accurate, just checking if defaultValue is truthy rather than if it equals null.

defaultValue ? ` [${defaultValue}]` : ""

@CLAassistant
Copy link

CLAassistant commented Nov 26, 2023

CLA assistant check
All committers have signed the CLA.

@@ -27,7 +27,7 @@ export const prompt: typeof globalThis extends { prompt: infer T } ? T
(globalThis as any)["prompt"] ??
function prompt(
message = "Prompt",
defaultValue = undefined,
Copy link
Contributor Author

@uncenter uncenter Nov 26, 2023

Choose a reason for hiding this comment

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

I'm pretty sure we don't need this since it would default to undefined either way right?

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@dsherret dsherret merged commit b8705b5 into denoland:main Dec 18, 2023
4 checks passed
@uncenter uncenter deleted the prompt-parity-with-deno branch December 18, 2023 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants