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

feat(outdated): interactive update #27812

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

nathanwhit
Copy link
Member

@nathanwhit nathanwhit commented Jan 24, 2025

WIP. This is currently just an impl for the UI part. Finishing this out should just be a matter of tweaking the UI and wiring up the inputs and outputs (though writing tests will be a tricky part)

@nathanwhit nathanwhit marked this pull request as ready for review January 31, 2025 19:03
out.flush()?;

Ok(())
}
Copy link
Member

@dsherret dsherret Jan 31, 2025

Choose a reason for hiding this comment

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

It might be better to switch this to https://github.com/dsherret/console_static_text with deno_terminal because then we don't need the crossterm dependency. Additionally, console_static_text is already used by the draw thread for progress bars: https://github.com/denoland/deno/blob/main/cli/util/draw_thread.rs and in the future we could hook it up so that you can do stuff like show progress bars at the same time as having interaction https://david.deno.dev/posts/console-static-text/

That said, we probably still want something for handling key presses though... maybe the repl has some code that does that? Otherwise I guess we need to use crossterm.

Copy link
Member Author

@nathanwhit nathanwhit Feb 1, 2025

Choose a reason for hiding this comment

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

The repl uses rustyline which has its own keypress handling AFAICT.

Did a quick check, this PR adds about 66 KiB to the binary - though not sure how much of that is crossterm. It does also add an extra version of mio to our dependency tree because we're on an old tokio version – though when we upgrade tokio it'll remove that duplication

Copy link
Member Author

Choose a reason for hiding this comment

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

For unix it probably wouldn't be too hard to roll our own, but no idea about windows

Copy link
Member

@dsherret dsherret Feb 1, 2025

Choose a reason for hiding this comment

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

I think at least here we should use deno_terminal and console_static_text to prevent having multiple implementations ending up in the executable. Maybe something for a follow-up though.

Copy link
Member

Choose a reason for hiding this comment

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

For deno_terminal, it would help us re-use the logic we have for NO_COLOR and TERM styling, but I think crossterm handles that as well: https://github.com/denoland/deno_terminal/blob/main/src/colors.rs

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.

Nice! LGTM, but I think we could take advantage of some of the functionality in our other crates more.

major: u64,
minor: u64,
patch: u64,
pre: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

Should we be using deno_semver::Version here? https://docs.rs/deno_semver/0.7.1/deno_semver/struct.Version.html

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.

2 participants