-
Notifications
You must be signed in to change notification settings - Fork 12.8k
"tsc --watch should clear screen on new compilation" Two: Electric Boogaloo #20389
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
"tsc --watch should clear screen on new compilation" Two: Electric Boogaloo #20389
Conversation
* added optional clearScreen method to System] * implemented via `x1Bc`, reset screen * fixes 13020
# Conflicts: # src/compiler/tsc.ts
src/compiler/watch.ts
Outdated
@@ -493,6 +493,10 @@ namespace ts { | |||
} | |||
|
|||
function updateProgram() { | |||
if (sys.clearScreen()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be if (sys.clearScreen)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should not use sys
here. you should use watchingHost.system
instead.
src/compiler/tsc.ts
Outdated
@@ -134,6 +134,10 @@ namespace ts { | |||
} | |||
|
|||
function reportWatchModeWithoutSysSupport() { | |||
if (sys.clearScreen()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see why you need this line here. checking in watch is sufficient.
you also need to accept the api baselines since you have changed the public API shape. |
For the tests, all the tests for watch are in https://github.com./Microsoft/TypeScript/blob/master/src/harness/unittests/tscWatchMode.ts |
After trying this out locally, I found it disruptive to have the screen immediately clear before the first "compilation succeeded" message. Removing that now and will add it to test cases. |
thanks @JoshuaKGoldberg for picking this up and driving it through. |
This is awesome, but really screws with output when multiple |
Fixes #20379
Port of #17756 since that hasn't been updated in a few weeks. The first iteration was written by @k0pernikus.
@mhegazy I like how the screen still has the "there's been a recompile" notice up top. Otherwise, for long recompiles, it can get a little confusing when the screen is completely blank for seconds at a time.
Re writing tests - I'm a little lost here. Is there somewhere this kind of test should go?
If it's acceptable to merge this PR without tests (which would be great for using the feature in nightlies!), I'd still be very interested in writing them later.
Todo: look into 2 baseline unit test failures for .d.ts filesTodo: tests, if this doesn't get merged in first (which is fine by me!)