-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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(watch): don't use alternate screen if clearScreen is set #2125
Conversation
bin/src/run/watch.ts
Outdated
const warnings = batchWarnings(); | ||
|
||
const initialConfigs = processConfigs(configs); | ||
|
||
const clearScreen = initialConfigs.some(config => config.watch.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.
just realized this iterator isn't quite right.
bin/src/run/watch.ts
Outdated
|
||
let screenWriter = screen.reset; | ||
configs = configs.map(options => { | ||
function processConfigs(configs: RollupWatchOptions[]): RollupWatchOptions[] { |
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.
processConfigs
was previously inside of start
bin/src/run/watch.ts
Outdated
@@ -71,15 +72,14 @@ export default function watch( | |||
code: 'UNKNOWN_OPTION' | |||
}); | |||
|
|||
if ( |
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'm not sure what potential side-effects are for this removal.
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.
Perhaps rather leave this in if you're unsure, since it seems like it wouldn't necessary apply to the use case?
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.
because processConfigs
was pulled out of the start
method, it no longer has access to the screenWriter
variable, especially since that also happens later on.
I may be able to store it in some other variable and then reset it to stderr in the start method when it gets called.
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.
Ah I see, if you can keep the existing behaviour it may be best not to remove something that seems so intentional, especially given this is untested code.
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.
Makes sense. Let me update the PR then.
Also, I'm not really sure how to add a test for this, any guidance would be appreciated. |
Hi, what needs to be done to get this merged? |
Hi @gkatsev, sorry for letting you wait that long. I just had another look through your PR and I must admit I have no idea how to add a test myself without adding some kind of mocking framework. From the code side I think this looks good. Even though it goes against my convictions I would actually be willing to merge this as it is without a test considering it is also a rather small change. Any objections from @guybedford ? |
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'm also not at all familiar with this part of the codebase, so would also suggest we merge and iterate if there are any problems.
bin/src/run/watch.ts
Outdated
@@ -71,15 +72,14 @@ export default function watch( | |||
code: 'UNKNOWN_OPTION' | |||
}); | |||
|
|||
if ( |
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.
Perhaps rather leave this in if you're unsure, since it seems like it wouldn't necessary apply to the use case?
@guybedford updated to keep |
@lukastaegert also, no worries, stuff happens and things fall through the cracks. |
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.
LGTM. Thanks @gkatsev for the follow-up here, appreciated.
Fixes #1804
This is the beginning of a PR to fixes #1804 based on some of the work that people did in that thread with minimal changes. Thanks @evocateur and @JimPanic
I think the changes are good but there's one thing that I'm not sure if there are any side-effects for but would love to get this merged in.