Skip to content
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

Handle keyboard input in child process #186

Closed
wants to merge 1 commit into from
Closed

Handle keyboard input in child process #186

wants to merge 1 commit into from

Conversation

webpro
Copy link

@webpro webpro commented May 4, 2022

When wiring release-it up with wireit and create a monorepo setup, I saw interesting use cases.

However, using a process like this would require a way to set stdio: 'inherit', or deal in some other way with keyboard input. Would you be open to something like this? Perhaps as a configuration option of sorts. Maybe this should even work out-of-the-box, although currently I'm not sure how handling this stream fits into the rest of the program.

Just to be sure, this first commit is not to be merged as-is, it's just to open this discussion in a draft PR.

Please see the readme and package.json of https://github.com/webpro/release-it-monorepo. Also see this screenshot for the working demo to publish three packages in this mini monorepo:

Screen Shot 2022-05-04 at 8 47 33 PM

By the way, release-it can be used in --ci mode as well, eliminating the need for user/keyboard input.

@google-cla
Copy link

google-cla bot commented May 4, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@webpro webpro changed the title Initial commit to discuss adding optional stdio: 'inherit' Handle keyboard input in child process May 4, 2022
@aomarks
Copy link
Member

aomarks commented May 5, 2022

Thanks for the PR! Definitely open to this or something along these lines, it's something I've been thinking we should support.

It seems like something that should be a setting. Perhaps like this?:

{
  "wireit": {
    "publish": "release-it",
    "interactive": true
  }
}

There are some details to think about:

  • What happens if two interactive processes try to run at the same time? Do we need to prevent this?
  • What happens if something depends on an interactive process?
  • What happens in watch mode?
  • Seems also related to Service mode #33.

@webpro
Copy link
Author

webpro commented May 5, 2022

Thanks for the PR! Definitely open to this or something along these lines, it's something I've been thinking we should support.

OK great!

It seems like something that should be a setting. Perhaps like this?:

{
  "wireit": {
    "publish": "release-it",
    "interactive": true
  }
}

There are some details to think about:

  • What happens if two interactive processes try to run at the same time? Do we need to prevent this?

Yes, at first thought it makes sense to me to basically enforce WIREIT_PARALLEL=1 for interactive: true. Maybe this is basically the same thing, if inherit would be the default for stdin.

  • What happens if something depends on an interactive process?

I guess that should be automatically solved as well by enforcing a single/sequential flow in interactive mode.

  • What happens in watch mode?

Uh, interesting. Not sure, tbh. Since watch is opt-in it's not an issue to be solved by wireit per se.

Using inherit for stdio might help there as well.

I'll update this PR now with a better fix to use inherit only for the stdin stream. This should also make the tests pass.

@webpro webpro marked this pull request as ready for review May 12, 2022 15:48
@webpro
Copy link
Author

webpro commented May 12, 2022

I just force-pushed the same (after the refactor between 0.3.1 and 0.4.1), would love to make progress here

@aomarks
Copy link
Member

aomarks commented May 12, 2022

I just force-pushed the same (after the refactor between 0.3.1 and 0.4.1), would love to make progress here

Sorry for the delay, I still need to think carefully through the implications of this and exactly how it should work, to answer some of the questions above, such as ensuring that only one interactive script at a time receives stdin. I'm currently working on #33 so we'll need to think about interaction there too.

@aomarks
Copy link
Member

aomarks commented May 12, 2022

Note that as a workaround, which I'm sure you've already discovered, you can probably just chain the build + release scripts in the regular npm script. There may not be any advantage to having wireit execute the final command -- right?

{
  "scripts": {
    "publish": "npm run build && release-it",
    "build": "wireit"
  }
}

@webpro
Copy link
Author

webpro commented May 13, 2022

I just force-pushed the same (after the refactor between 0.3.1 and 0.4.1), would love to make progress here

Sorry for the delay, I still need to think carefully through the implications of this and exactly how it should work, to answer some of the questions above, such as ensuring that only one interactive script at a time receives stdin. I'm currently working on #33 so we'll need to think about interaction there too.

Alright, that's cool of course. No hurries, I'm just excited for this feature :)

Note that as a workaround, which I'm sure you've already discovered, you can probably just chain the build + release scripts in the regular npm script. There may not be any advantage to having wireit execute the final command -- right?

{
  "scripts": {
    "publish": "npm run build && release-it",
    "build": "wireit"
  }
}

Yes, I'm aware of this. I'm especially interested in this specific feature, so users of release-it can use it for monorepos too. I think adding (something like) the changes in this PR can result in a great combination of both tools:

{
  "scripts": {
    "release": "wireit"
  },
  "wireit": {
    "release": {
      "command": "release-it --no-npm",
      "dependencies": [
        "./packages/bravo:release",
        "./packages/mol:release",
        "./packages/zen:release"
      ]
    }
  }
}

This monorepo serves as a demo. Without wireit, this does the same:

{
  "scripts": {
    "release": "npm --prefix packages/bravo run release && npm --prefix packages/bravo run release && npm --prefix packages/zen run release && release-it --no-npm"
  }
}

Another option would be to build something like this into release-it itself, but that would add complexity that I'd rather prevent. I haven't found great alternatives myself so far, but when wireit ends up without supporting interactive mode I'll definitely consider that.

I'd also like to add that roughly 50% of release-it runs happen in CI mode, so without interactivity, which works like a charm today (even without WIREIT_PARALLEL=1, although output is obviously mixed up as a consequence).

@aomarks
Copy link
Member

aomarks commented Nov 7, 2022

Thank you for this PR!

I'm still planning to support interactive scripts at some point, but I think there's more design work to do.

It can't be as simple as forwarding all stdin (as this PR does), because if there are multiple scripts running at the same time that take stdin, it won't be clear which one is "focused".

We also need to disallow interactive scripts from ever being cached, because the input provided by the user won't be tracked, so we can't have a good cache key.

I expect we'll need to add an "interactive": true setting, which will ensure that only one interactive script is active at a time.

Tracking at #281

@aomarks aomarks closed this Nov 7, 2022
@webpro
Copy link
Author

webpro commented Nov 7, 2022

Yes, that's totally clear and fair. Thanks!

@webpro webpro deleted the feature/stdio-inherit branch November 7, 2022 18:41
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