Skip to content

Do not open alternate buffer during piped --watch - #1574

Merged
Rich-Harris merged 2 commits into
rollup:masterfrom
evocateur:altscreen-optional
Aug 27, 2017
Merged

Do not open alternate buffer during piped --watch#1574
Rich-Harris merged 2 commits into
rollup:masterfrom
evocateur:altscreen-optional

Conversation

@evocateur

Copy link
Copy Markdown
Contributor

Recently, the output of rollup --watch was modified to reset the buffer when changes caused a rebuild. Works brilliantly when invoked in an active terminal, e.g. rollup -cw.

However, this really doesn't play nicely with processes that pipe the output from a child process (such as lerna exec), which is effectively a non-interactive terminal. Clearing the screen repeatedly causes a bunch of thrashing, as well as unwanted removal of other process logging.

Tests?!

I was unable to locate examples of testing both CLI and watching, so I did a bunch of local testing with npm-linked rollup in several lerna repos where I was using rollup. Here's an example repo I whipped up that demonstrates the problem and solution:

https://github.com/evocateur/lerna-rollup-example

When executed in a child process, resetting the alternate buffer causes too much thrashing.

- Added explicit ansi-escapes to devDependencies
- Moved alternate buffer actions to separate file
- Omits "waiting for changes..." message when piped
@evocateur

Copy link
Copy Markdown
Contributor Author

Holy cow, that AppVeyor build has been running for over 30 minutes on node 4. That's not ideal :/

This ensures that the alternate buffer is correctly closed, regardless of exit method.

- Add signal-exit to devDependencies
- Avoid hooking stdin 'end' event if stdin is _not_ piped
- Remove all listeners during close()
@Rich-Harris
Rich-Harris merged commit d3cfa05 into rollup:master Aug 27, 2017
@Rich-Harris

Copy link
Copy Markdown
Contributor

aaaahhhh, that's much cleaner. Thank you 👍

@evocateur

evocateur commented Aug 28, 2017 via email

Copy link
Copy Markdown
Contributor Author

@evocateur
evocateur deleted the altscreen-optional branch August 29, 2017 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants