-
-
Notifications
You must be signed in to change notification settings - Fork 610
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 rendering when terminal is erased on first render #586
base: master
Are you sure you want to change the base?
Conversation
Not sure why tests are failing on CI, while passing locally consistently. Will need to look into it. Help also appreciated to ship this fix faster! |
You can use https://github.com/nektos/act to debug the ci |
<comment deleted as some testing revealed it didn't work> |
@vadimdemedes strangely with this fix, tests pass locally if I run The reason for that fix is that in CI we don't clear the terminal, we return earlier in the code, so it makes sense that the check for |
Hey there, wondering if there are any updates on this work. I have a need on this being resolved. Is there anything I can do to help get this over the finish line? :) |
@DaniGuardiola unfortunately I haven't had the time to continue on this bug, the issue as I left it was one with tests. The code was working fine. CI is failing while local tests are passing, but I couldn't find the root cause. If you'd like to pick it up you can also check what I tried to do in a separate branch to maybe get some ideas. |
@DaniGuardiola The weird issue is that I fixed the original issue in this PR and confirmed it works on my laptop, but when I submitted this PR, it consistently fails on CI. Huge help would be to figure out what's going on in CI here to unblock this PR. |
I changed my approach and this stopped being a blocker for me. I don't think I'll have the time to look into it, sorry. |
Fixes #585.
@matteodepalo did a great job documenting the issue, so I'm not going to repeat it here.
Context
Ink has two ways of rendering output, depending how tall it is:
If output is shorter than terminal height, Ink erases the lines from last output and writes lines from the new output. Ink tracks how many lines did the last output have for this to work. This is handled in https://github.com/vadimdemedes/ink/blob/master/src/log-update.ts, which is a fork of
log-update
package.If output is equal or taller than terminal height, Ink erases the entire terminal history (or scrollback as it's commonly referred to) and writes new output. Note that Ink doesn't need to track how many lines to erase here, because it just erases the entire contents of the current terminal.
ink/src/ink.tsx
Lines 176 to 182 in 8a04760
Root cause
The key insight here is this:
In #585, the very first render starts with the second rendering method and erases the terminal completely. On next render, the first method kicks in, but there's no information how many lines to erase, since second method doesn't store that information.
That's why the output from the first render doesn't get erased, since for
logUpdate
this variable is zero.ink/src/log-update.ts
Line 12 in 8a04760
Solution
This PR fixes this bug by separating the write operation that erases the terminal and writes the output. This way, even if the entire terminal is erased,
logUpdate
is still used to render the output and keep track of how many lines were written.I've also added
force
option tologUpdate
render function to force it to always render the string we give to it without bailing out if it equals to the last output. This can happen whenStatic
output changes, but normal output isn't.ink/src/log-update.ts
Lines 23 to 25 in 8a04760
I've also added
erase
option tologUpdate
render function, so that we can tell it to not do erasing of its own, if we erased the entire terminal already.