feat: skip stdin resuming in watch mode - #2410
Conversation
|
I think there are a few questions that need answering before this is merged:
It would appear that it could be overridden: → node
> process.stderr.isTTY
true
> process.stderr.isTTY = false
false
> process.stderr.isTTY
false |
|
It sounds strange. Why do I need to override something if I already run rollup in watch mode? I think that the issue may be treated as a bug. Watch mode is not running correctly in different environments. So, I think a user should use normal mode when it is needed instead of relying on this check. |
That makes the assumption that all environments are equal. But that is not the case, and not a correct assumption.
There are differences in what the Node API will support between environments that are TTY and non-TTY. It's an important distinction when dealing with console output. This commit 0295c5b implemented the check you are now seeking to revert, and from the code it looks like there was specific intent to that addition. The fact of the matter here is that executing this in a non-TTY environment can have unpredictable consequences. There's no reason that every executable out there should test their environment for support API when the In my opinion (other maintainers may feel differently) the end goal of this PR would be better suited as a contribution to the Lerna project, to simulate TTY support. Or the suggested workarounds in local setups. I don't believe this is a good change that will benefit users as a whole; conversely, only a select niche of users, and will likely introduce a regression for other users. You can read more about the differences in TTY and non-TTY environments here: https://books.google.com/books?id=FZcQAwAAQBAJ&pg=PA73&lpg=PA73&dq=node+istty&source=bl&ots=P9Ox_cheVQ&sig=ZR9t_p-AIC2_W1Jk04Hz0zKhosQ&hl=en&sa=X&ved=2ahUKEwjLy6eN4u_cAhWGtlMKHTOhDy44ChDoATABegQICRAB#v=onepage&q=node%20istty&f=false |
|
|
||
| case 'END': | ||
| if (!silent && isTTY) { | ||
| if (!silent) { |
There was a problem hiding this comment.
It seems this check was initially added exactly for TTY support - #1574.
Can you perhaps explain why you think this should be the other way around?
There was a problem hiding this comment.
Sorry, I mean this line was initially added exactly for Lerna support :)
There was a problem hiding this comment.
| if (!process.stdin.isTTY) { | ||
| process.stdin.on('end', close); // in case we ever support stdin! | ||
| process.stdin.resume(); | ||
| } |
There was a problem hiding this comment.
Is this a bug fix, independent of the above? Or are both changes needed for the lerna support?
|
Lerna should work out of the box definitely, would be great to follow what is needed here further. |
|
I found the original PR that broke my project: #1774. This causes the 'end' event to be triggered in lerna. So rollup stops watching changes. In webpack a such check is in condition that becomes true when specific option is set: https://github.com/webpack/webpack-cli/blob/master/bin/cli.js#L516. But I didn't find this option in the webpack docs. However, the default value of this option is false. |
|
@kvet thanks for tracking this down, this seems good to me. As for the use case of rollup/rollup-watch#30, surely that is just Regardless I think we should merge this then come back to that. |
guybedford
left a comment
There was a problem hiding this comment.
Thanks for making it minimal!
lukastaegert
left a comment
There was a problem hiding this comment.
Not 100% sure this is the proper solution but I agree that having Lerna working is probably more important.
Fix react#8685 Refer same issue with rollup rollup/rollup#2410 (comment)
This is necessary when rollup is used as the front end for an Elixir Phoenix application. In dev mode, the phoenix watchers run the rollup command as a child process and the underlying erlang port mechanism assumes that the child process closes when the parent stdin is closed. Without this fix the old zombie processes never close and accumulate as you stop and start in development mode. This was discussed and fixed in 2017 in the original rollup-watcher repository: * rollup/rollup-watch#30 (comment) * https://github.com/rollup/rollup-watch/pull/57/files It appears to then have been removed as a side effect of another change * rollup#2410 Finally, was requested back in a PR that never got merged because of some doubt around the need to hide it behind a CLI flag * https://github.com/rollup/rollup/pull/2653/files For reference, webpack implements this hidden behind the --watch-stdin CLI flag.
* Ensure --watch mode exits correctly when stdin is closed. This is necessary when rollup is used as the front end for an Elixir Phoenix application. In dev mode, the phoenix watchers run the rollup command as a child process and the underlying erlang port mechanism assumes that the child process closes when the parent stdin is closed. Without this fix the old zombie processes never close and accumulate as you stop and start in development mode. This was discussed and fixed in 2017 in the original rollup-watcher repository: * rollup/rollup-watch#30 (comment) * https://github.com/rollup/rollup-watch/pull/57/files It appears to then have been removed as a side effect of another change * #2410 Finally, was requested back in a PR that never got merged because of some doubt around the need to hide it behind a CLI flag * https://github.com/rollup/rollup/pull/2653/files For reference, webpack implements this hidden behind the --watch-stdin CLI flag. * Add a test that times out without this change Co-authored-by: Lukas Taegert-Atkinson <lukas.taegert-atkinson@tngtech.com>
This PR contains:
Breaking Changes?
If yes, please describe the breakage.
Please Describe Your Changes
Solving the following problem: #1919