retypeapp / retype

Retype is an ✨ ultra-high-performance✨ static site generator that builds a website based on simple text files.
https://retype.com
Other
1.07k stars 204 forks source link

Disruptive CLI behavior #103

Closed DannyBen closed 3 years ago

DannyBen commented 3 years ago

So, I have tried to overlook this little annoyance, thinking it's a matter of taste, but now it has become too disruptive in my workflow, and I believe requires some attention.

There are two issues with the retype build command (and possibly others - if it matters, I can test further).

Annoyance

The fact that the command clears the screen is highly unusual (at least in a linux terminal). I know of no other tool that does that, and even if there is - for each one, there are a hundred that don't do it. I think this behavior should be removed. Just print status to screen, plain and simple. Colors are ok, Unicode icons are somewhat ok, but don't take over my screen, please.

Bug

I am not sure if you can reproduce it or not, but for me this reproduces consistently. It might be related to the first issue.

It seems like you are sending another control signal to the terminal (perhaps the same clear signal does that?). This signal removes a certain configuration I have in my terminal.

I have the following snippet in my ~/.inputrc file:

# Change up/down arrows behavior
"\e[A": history-search-backward
"\e[B": history-search-forward

This makes it so typing a partial command, and then using up arrow, causes the terminal to look in the history for the latest matching command.

After running retype build - this functionality is removed. I have to shut the terminal and reopen it if I want to have the functionality again.

I have tried reproducing it using an easier method for your benefit, but without success.

I am using Ubuntu 18 in a Vagrant guest on a Windows 10 host, and using PuTTY as the terminal software, but I suspect it can be reproduced in a less specific setup.

geoffreymcgill commented 3 years ago

Thanks for the detailed reports. We will take a good look at these.

I also noticed and don't like that the console clears with each run. I'm not sure what's doing that, but we will investigate.

@fabriciomurta Can you reproduce the Bug where it's removing the custom terminal config(s)?

DannyBen commented 3 years ago

Here is a screencast of the bug if it helps:

  1. At first I am typing a partial command and up/down arrows
  2. Then running retype build
  3. Then showing the arrows behavior no longer works the same. screencast
fabriciomurta commented 3 years ago

re: Annoyance

This can really break the flow of the terminal, as a simple retype --version clears the screen and it may become hard to scroll up and see the context where the retype command was called due to the term control codes' influence.

re: Bug

This issue is not reproducible using cygwin. After running Retype the arrow keys still searches for what's typed.

But I could reproduce the issue on macos.

This is possibly related to Retype sending control characters to the terminal and just removing the clear screen instruction might not really avoid the issue, we'll keep an eye open for that. When this happens you can reset your terminal just by calling reset or echo -e "\033c". You can have retype not trigger this by redirecting or piping its output, so retype --version | cat result="$(retype --version)" retype --version > retype.log

It will take a bit more in depth investigation to get what exact control code retype is emitting, as it simply doesn't emit any control code if it knows we are capturing its output.

geoffreymcgill commented 3 years ago

We are removing the clear command and will include the change in v1.8.0.

DannyBen commented 3 years ago
  1. I am glad it is reproducible.
  2. I noticed that even retype --version also kills the terminal arrow behavior.
  3. I can confirm reset helps.
  4. I wonder why does retype need to send any control characters to my terminal.
fabriciomurta commented 3 years ago

Further investingating the issue with ~/.inputrc, turns out this:

dotnet new console
dotnet run

is all one needs to trigger it. I am afraid best place to log this issue would be, perhaps, in dotnet/command-line-api repository. I tried some flavors of console output with no luck.

Again, redirecting output in any way avoids the issue.

If this really really bugs you, add this to your .bashrc:

function() retype() {
    retype "${@}" | cat
}

This is going to "wash away" any colors retype would print to the console though. Basically yellow Warning labels and red Error ones.

DannyBen commented 3 years ago

Thanks....

I am afraid best place to log this issue would be, perhaps, in dotnet/command-line-api repository

Another notch in my "Microsoft does not understand developers" belt.

Again, redirecting output in any way avoids the issue.

This is not really a long term solution. I prefer just running reset afterwards.

I am not really familiar with dotnet - doesn't it just have a simple print to terminal? Or do you have to go through a new console instance to do so?

As a long time web developer, I can tell you that we refer to Microsoft as "the gift that keeps on giving".... (think Internet Explorer).

fabriciomurta commented 3 years ago

Here's the actual capture of a terminal where I run a simple echo and the default console app template:

mac:test user^M^M
$ echo 'Hello World!'^M
Hello World!^M
mac:test user^M^M
$ dotnet run^M
^[[?1h^[=^[[?1h^[=^[[?1h^[=^[[?1h^[=Hello World!^M
^[[?1h^[=mac:test user^M^M
$

Here you can see all "shady characters" it sends.

Another notch in my "Microsoft does not understand developers" belt.

Basically their functions and cconsole handling were done for windows consoles and has evolved a different path that unix's xterm, so while these escape sequences probably, and hopefully make sense there, it simply came as a left over to linux -- or maybe it was implemented to solve some other linux/bsd issues that raised during the "multi-platform" port.

I am not really familiar with dotnet - doesn't it just have a simple print to terminal? Or do you have to go through a new console instance to do so?

The simpe "console app template" is as big as an echo of that hello world string. There will probably be some ways to access console and show output directy but then the "portable console output" aspect of dotnet would be gone. Well, as portable as we can tell...

As a long time web developer, I can tell you that we refer to Microsoft as "the gift that keeps on giving".... (think Internet Explorer).

The bright side now is, we do have an actual repository and open code source we can blame and even propose a fix, instead of being stuck with a phone call without answers, an ever-open ticket and black box sources. So there are at least hopes this can be fixed. As far as I can tell, they needed to wrap specific unix versions of at least come System.Console.* source files for specifics; thus a fix is pretty feasible. I couldn't see any report about that and encourage you to try and log an issue there. It should be a matter of time before somebody else being hit by this issue shows up in their boards. Probably already done, just nobody who created an issue to their CLI interface.

On a second thought... I was wrong with where to report the issue. It is not a dotnet CLI issue; retype is affected by this for a reason. That's actually microsoft's System.Console implementation issue.... And in fact, we got dotnet/runtime#27626 which really really looks like the issue. They just didn't mention inputrc but that "left state" makes sense to place inputrc settings "afloat".

fabriciomurta commented 3 years ago

...okay, there seems to be a solution!

Can you tell me if it helps you:

DannyBen commented 3 years ago

Thanks for the detailed analysis.

I [...] encourage you to try and log an issue there

I am usually opening an issue on things I understand, and am their direct user, since I will be unable to debug and respond to any followup questions in cases where I am a "user by proxy".

To me, this is a retype issue, and for retype it's a dependency issue. I understand if you don't wish to pursue this further, but I hope that you do, since this is a dependency that degrades the experience of your product, even if just for that one annoying user... :)

fabriciomurta commented 3 years ago

In case the little recipe above works for you it will be very possible for Retype to apply the solution and I will pronto submit a pull request for our team to evaluate it.

DannyBen commented 3 years ago

Can you tell me if it helps you:

Checked. Yes - this works. Are you thinking about echoing it inside retype after each command? If so, just also consider the Interrupt signals on the server, probably need to echo it inside trap or the dotnet equivalent.

fabriciomurta commented 3 years ago

Thanks for the feedback!

We should be able to configure the terminal to keyboard local as long as retype has the control of the program to print the control characters. In some circumstances (signals KILL, STOP) it won't do. If the program fails before retype gains control, too. But hopefully this should fix the vast majority of situations where the terminal would be broken.

DannyBen commented 3 years ago

In some circumstances (signals KILL, STOP) it won't do

Hmm... so you do intend to trap at least the INTERRUPT signal, so that Ctrl+Cing out of retype server and retype watch will also restore normal behavior, yes? Or is this also a signal that cannot be trapped?

fabriciomurta commented 3 years ago

sigint (ctrl+c) and sigterm ("graceful" kill) can be trapped. I'm not sure about trapping all other possible signals though, will probably leave them alone as many of them won't even interrupt the program flow.

DannyBen commented 3 years ago

sigint (ctrl+c) and sigterm ("graceful" kill) can be trapped

sounds good.

geoffreymcgill commented 3 years ago

I see https://github.com/dotnet/runtime/issues/27626 is also referenced by a larger redesign task within https://github.com/dotnet/runtime/issues/52374. Looks like the dotnet/runtime team are aware of the issues and hopefully will be improving.

Any changes in the dotnet runtime are likely to take a while, so right now, we will try our best to workaround the issues.

We will also monitor issue https://github.com/dotnet/runtime/issues/27626 + https://github.com/dotnet/runtime/issues/52374 and @fabriciomurta will provide them with a simplified sample demonstrating how to reproduce the scenario discussed here. Hopefully that helps bump the issue and priority.

fabriciomurta commented 3 years ago

Unfortunately, making retype output the "enter-local-mode" control sequence was not helpful, as .NET Runtime will always append a last "enter-application-mode" sequence last thing in the console app's output. :(

So the fix I was thinking about won't help at all.

DannyBen commented 3 years ago

So the fix I was thinking about won't help at all.

That is unfortunate. But at least, thanks to your echo tip, I have configured my alias manager to have

retype: retype "$@" ; echo -e "\033[?1l"

so it will be a little less "in my face".

fabriciomurta commented 3 years ago

Yet another update. It was possible to apply the fix to the NPM version of Retype. This means, if you are installing retype via npm install --global retypeapp and running the retype this places on path, you should no longer face the inputrc issue in the next (1.8.0) Retype release.

But the issue will still happen for who uses the dotnet tool install --global retypeapp version.

Additionally, windows is not affected by the issue, and whenever retype output is redirected, these control characters are not printed, so the issue is also not applicable.

To keep the NPM wrapper simple, we didn't implement "signal traps"; but there should be very very little room for the signals to actually affect the wrapper (although possible). Problem is that the wrapper itself simply can't (portably) tell the terminal mode has been switched. But let us know if you are hitting this scenario too often and we'll think on something.

fabriciomurta commented 3 years ago

Oh and it seems we won't be clearing screen anymore starting by 1.8.0. But now we can't close this issue as you also reported the bigger bug altogether.

Anyways, great feedback and keep them coming!

DannyBen commented 3 years ago

I installed the npm version (with yarn though).

to keep the NPM wrapper simple, we didn't implement "signal traps";

so doesn't this mean that retype watch and retype run will still break the terminal?

Oh and it seems we won't be clearing screen anymore starting

That's great!

But now we can't close this issue as you also reported the bigger bug altogether.

Are you rea-hea-healy encouraging me to open more issues?... :)

Jokes aside, I thought these will end up being the same problem, and wanted to be careful about my "environmental footprint" in the retype issues/discussions.

fabriciomurta commented 3 years ago

so doesn't this mean that retype watch and retype run will still break the terminal?

sight you're right, where was I with my head when I tried to just send signals to the inner process instead of going such a simple (and very common) path?

We already have a solution for that underway, hopefully making it to 1.8. Thanks. Basically we needed to ignore ctrl+c, or else even further NodeJS terminal cleanup will be missed (that we cannot just wire back in), and the issue would become much worse.

geoffreymcgill commented 3 years ago

@DannyBen I've sent you an email with instructions on how to test the most recent preview build of v1.8.0.

DannyBen commented 3 years ago

Confirmed in version 1.7.1-202107290616

This is much more streamlined and cleaner, well done.

DannyBen commented 3 years ago

I am guessing this can be closed as well, I hope you don't mind my housekeeping OCD.

fabriciomurta commented 3 years ago

Not really, the bug is only "worked around" for NPM users. The dotnet tool audience is still affected by the issue. In fact, NPM is but using a work-around the issue until .NET runtime addresses that... But that should take a long long time. We will still keep our internal issue open so we don't forget about that.