Skip to content
This repository has been archived by the owner on Jun 15, 2023. It is now read-only.

Re-enable brunch to run in background #998

Closed
shreeve opened this issue Jul 27, 2015 · 13 comments
Closed

Re-enable brunch to run in background #998

shreeve opened this issue Jul 27, 2015 · 13 comments

Comments

@shreeve
Copy link
Contributor

shreeve commented Jul 27, 2015

Issue #922 caused brunch to no longer run in the background. We used to run it and be able to press Ctrl+Z and bg to have it run in the background. After #922, we see "stopped" and brunch refuses to run in the background. If we back out #922, everything is cool.

Is there a quick fix for this?

@paulmillr
Copy link
Member

@josevalim what was your issue with gh-922? Did it cause any actual harm?

@josevalim
Copy link
Contributor

@paulmillr we need it otherwise if we start brunch from inside another process (using something like popen), it won't ever shutdown if the parent process dies. Most UNIX command line tools behave like that.

We could, however, make this or the daemon behaviour configurable. For example, if pass brunch watch --daemon, we won't check for STDIN. Or, alternatively, only brunch watch --popen would check STDIN (or a more descriptive flag).

@es128
Copy link
Member

es128 commented Jul 28, 2015

Here's the discussion regarding why that was needed: #920

@es128
Copy link
Member

es128 commented Jul 28, 2015

So either there is some other way to run a process in the background that doesn't close stdin, or we need to create an option that disables this behavior.

@shreeve
Copy link
Contributor Author

shreeve commented Jul 31, 2015

Since this method of operation is "new" (at least for brunch), perhaps the simplest way to enable it is similar to @josevalim's earlier example:

$ cat -
hello
CTRL-D

Basically, we'd just add the ability to listen to stdin by using the same - option as above.

@es128
Copy link
Member

es128 commented Jul 31, 2015

@shreeve did you try it out and confirm that works for you? Would rather add something to the docs regarding how to do that than add an option.

@shreeve
Copy link
Contributor Author

shreeve commented Jul 31, 2015

I think to support both modes (ie - the original style and also the style suggested by @josevalim), we will need to add an option regardless. In this case, my guess is the simplest option is to allow the - option to mean (as it does in many other utlities) that brunch should read from stdin. So, the fix would be something like this:

  if config.read_stdin
    process.stdin.on 'end', -> process.exit(0)
    process.stdin.resume()

and then setting config.read_stdin if the - option is passed. By default it is not passed (this is how we've been using brunch historically), but if you'd like the feature (as @josevalim does), then you just pass - (again, similar to many other unix utils).

@es128
Copy link
Member

es128 commented Jul 31, 2015

Ok now I think I understand your suggestion better.

Commander might get in the way of enabling the option by passing the -, but I'll check into it. Ultimately it would just be sugar on top of the more explicit config option.

@shreeve
Copy link
Contributor Author

shreeve commented Jul 31, 2015

It looks like it works with commander.

  .option('-, --stdin', 'Read from stdin')

@es128
Copy link
Member

es128 commented Jul 31, 2015

Cool! If you've gone this far, perhaps you'd like to just send a PR?

@josevalim
Copy link
Contributor

👍 for --stdin. We could support - but it is probably unnecessary, as I doubt people will be invoking this directly but rather as part of another software.

@paulmillr
Copy link
Member

Could we also make the option "hidden" by default? To avoid unnecessary confusion.

@josevalim
Copy link
Contributor

Folks, we are close to releasing Phoenix 1.0, so if this is going to change, it would be nice it it was soon. I will send a pull request today.

EDIT: s/should/it would be nice :D

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

4 participants