fix(watch): don't use alternate screen if clearScreen is set - #2125
Conversation
|
|
||
| const initialConfigs = processConfigs(configs); | ||
|
|
||
| const clearScreen = initialConfigs.some(config => config.watch.clearScreen); |
There was a problem hiding this comment.
just realized this iterator isn't quite right.
|
|
||
| let screenWriter = screen.reset; | ||
| configs = configs.map(options => { | ||
| function processConfigs(configs: RollupWatchOptions[]): RollupWatchOptions[] { |
There was a problem hiding this comment.
processConfigs was previously inside of start
| code: 'UNKNOWN_OPTION' | ||
| }); | ||
|
|
||
| if ( |
There was a problem hiding this comment.
I'm not sure what potential side-effects are for this removal.
There was a problem hiding this comment.
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.
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.
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.
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 ? |
guybedford
left a comment
There was a problem hiding this comment.
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.
| code: 'UNKNOWN_OPTION' | ||
| }); | ||
|
|
||
| if ( |
There was a problem hiding this comment.
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. |
guybedford
left a comment
There was a problem hiding this comment.
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.