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.

Sign up for GitHub

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

Jump to bottom

avoid ANSI clearScreen #7523

Merged
merged 2 commits into from
Dec 24, 2018
Merged

avoid ANSI clearScreen #7523

merged 2 commits into from
Dec 24, 2018

Conversation

jeysal
Copy link
Contributor

@jeysal jeysal commented Dec 17, 2018

clearScreen is quite aggressive and can e.g. brick the user's shell theme.
Better use eraseScreen and cursorTo(0, 0) instead.

Summary

Pattern prompts and interactive snapshot mode use ansiEscapes.clearScreen, which can sometimes clear more than it should - in this case the shell theme (note the background color changing from dark gray to black when pressing p in watch mode):
Before
I can reproduce this behavior running bash & zsh inside kitty on macOS & Linux with the minimal base16-shell theming setup as described in its readme Installation/Configuration sections.

Test plan

The combination of eraseScreen and cursorTo(0, 0) achieves the same goal as clearScreen, but does not exhibit its problems:
ezgif-4-c3412a9b4d8b

Open question:
How can we guard against regressions that introduce new usages of clearScreen? Introduce an abstraction on top of ansi-escapes and avoid direct usage? Seems a bit excessive

@SimenB SimenB requested a review from rogeliog December 18, 2018 01:04
@SimenB
Copy link
Member

SimenB commented Dec 18, 2018

Should we be using https://github.com/facebook/jest/blob/54f4d4ebd3d1a11d65962169f493ce41efdd784f/packages/jest-cli/src/constants.js#L12 instead?

@jeysal
Copy link
Contributor Author

jeysal commented Dec 18, 2018

That seems to behave properly as well. But I think we'd have to move it, since we don't usually import from jest-cli? Any suggestions where we could define this constant?

@SimenB
Copy link
Member

SimenB commented Dec 18, 2018

jest-util seems reasonable 🙂

@jeysal jeysal closed this Dec 19, 2018
@codecov-io
Copy link

Codecov Report

Merging #7523 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7523   +/-   ##
=======================================
  Coverage   67.73%   67.73%           
=======================================
  Files         247      247           
  Lines        9513     9513           
  Branches        5        5           
=======================================
  Hits         6444     6444           
  Misses       3067     3067           
  Partials        2        2

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9208ac6...9208ac6. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Dec 19, 2018

@jeysal hey, a rebase gone wrong? 🙂

@jeysal
Copy link
Contributor Author

jeysal commented Dec 19, 2018

Working on it rn, looks like GitHub closed the PR because I force pushed after resetting to origin/master.
Nothing lost though, no worries :)

@jeysal
move specialChars to jest-util
fbc7bb0
@jeysal
avoid ANSI clearScreen
ed7a456 
clearScreen is quite aggressive and can e.g.
brick the user's shell theme.
@jeysal jeysal reopened this Dec 19, 2018
@jeysal
Copy link
Contributor Author

jeysal commented Dec 19, 2018

Alright, ready for review. Seems like I managed to totally confuse Travis & AppVeyor though :D

SimenB
SimenB approved these changes Dec 20, 2018
View reviewed changes
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

This is great!

@SimenB SimenB requested a review from thymikee December 20, 2018 00:08
thymikee
thymikee approved these changes Dec 20, 2018
View reviewed changes
Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@SimenB SimenB merged commit 00603d1 into jestjs:master Dec 24, 2018
@jeysal jeysal deleted the avoid-ansi-clearscreen branch December 26, 2018 20:55
@rogeliog rogeliog mentioned this pull request Mar 13, 2019
Merged
captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019
@jeysal @captain-yossarian
avoid ANSI clearScreen (jestjs#7523)
297bd6e 
* avoid ANSI clearScreen

clearScreen is quite aggressive and can e.g.
brick the user's shell theme.

* move specialChars to jest-util
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@thymikee thymikee thymikee approved these changes

@SimenB SimenB SimenB approved these changes

@rogeliog rogeliog Awaiting requested review from rogeliog

Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jeysal @SimenB @codecov-io @thymikee @facebook-github-bot