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

C3: Fix disappearing cursor after early exit #4087

Merged
merged 3 commits into from
Oct 3, 2023
Merged

Conversation

jculvey
Copy link
Contributor

@jculvey jculvey commented Oct 3, 2023

Fixes #3835.

What this PR solves / how to test:

This solves an issue where exiting c3 early via ctrl+c causes the terminal cursor to become hidden.

Associated docs issue(s)/PR(s):

  • [insert associated docs issue(s)/PR(s)]

Author has included the following, where applicable:

Reviewer is to perform the following, as applicable:

  • Checked for inclusion of relevant tests
  • Checked for inclusion of a relevant changeset
  • Checked for creation of associated docs updates
  • Manually pulled down the changes and spot-tested

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

@jculvey jculvey requested review from a team as code owners October 3, 2023 04:22
@changeset-bot
Copy link

changeset-bot bot commented Oct 3, 2023

🦋 Changeset detected

Latest commit: 3fed1fa

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

This PR includes changesets to release 1 package
Name Type
create-cloudflare 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

@jculvey jculvey changed the title C3 disappearing cursor C3: Fix disappearing cursor after early exit Oct 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2023

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6395573058/npm-package-wrangler-4087

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6395573058/npm-package-wrangler-4087

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6395573058/npm-package-wrangler-4087 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6395573058/npm-package-cloudflare-pages-shared-4087

Note that these links will no longer work once the GitHub Actions artifact expires.


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare 3.20230922.0 3.20230922.0
workerd 1.20230922.0 1.20230922.0
workerd --version 1.20230922.0 2023-09-22

|

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@dario-piotrowicz
Copy link
Member

When you use C3 the cursor disappears (in some paths, especially the framework ones, I don't think this happens always always), this makes it inconvenient for example to modify the app name you're providing.

In a conversation we had a while back I understood that the disappearance of the cursor in C3 is not intentional.

In this PR your are making it re-appear after C3 terminates if the user cancels the operation, this does make things better but I don't think this is the proper solution, shouldn't we fix the issue at the root and not have the cursor disappear in the first place?

@jculvey
Copy link
Contributor Author

jculvey commented Oct 3, 2023

When you use C3 the cursor disappears (in some paths, especially the framework ones, I don't think this happens always always), this makes it inconvenient for example to modify the app name you're providing.

It happens whenever you are in the middle of a clack select or confirm prompt and you cancel.

In a conversation we had a while back I understood that the disappearance of the cursor in C3 is not intentional.

In this PR your are making it re-appear after C3 terminates if the user cancels the operation, this does make things better but I don't think this is the proper solution, shouldn't we fix the issue at the root and not have the cursor disappear in the first place?

The root cause is that clack hides the cursor via ansi escape codes whenever a select or confirm prompt is rendered. It does this for good reason, since having a cursor in addition to the artificial cursor affordance we give would be confusing. Imagine having another cursor in this select list:

image

Since we're using custom prompts instead of the built-in prompts that clack provides (due to us wanting to have more fine grained control over the rendering step), it's on us to handle restoring the cursor state. This change does it almost identically to how clack handles it.

@jculvey jculvey force-pushed the c3-disappearing-cursor branch from 2af0669 to 3fed1fa Compare October 3, 2023 16:09
@dario-piotrowicz
Copy link
Member

@jculvey ah ok I see, thanks a bunch for the explanation ❤️

If that's the case I'd imagine that then this solution is ok, but I still think that there's more to do regarding the cursor?

As I mentioned it looks like the cursor is not even showing when you're providing a name/directory for your app:
Screenshot 2023-10-03 at 17 19 38

anyways if that's a different thing and is not conflicting with the current solution then I'm ok with going forward with this (and hopefully handle the broader cursor issue later on?)

@jculvey jculvey merged commit 57e9f21 into main Oct 3, 2023
@jculvey jculvey deleted the c3-disappearing-cursor branch October 3, 2023 17:08
@workers-devprod workers-devprod mentioned this pull request Oct 3, 2023
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.

🐛 BUG: [C3] Early exit causes cursor in terminal to disappear
3 participants