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

CLI: Refactor package manager methods to be async #22401

Merged
merged 2 commits into from May 8, 2023

Conversation

yannbf
Copy link
Member

@yannbf yannbf commented May 4, 2023

Relates to #22343

What I did

This PR turns most of the package manager methods into async, plus updates every other place that refers to them.
It also does a small refactoring on some method signatures to make the code more readable and maintainable.

This is to prepare for other PRs which will need the async nature of the package manager proxies!

How to test

  1. Tests should be green (unit + sandbox)

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@yannbf yannbf added maintenance User-facing maintenance tasks cli labels May 4, 2023
@yannbf yannbf requested a review from ndelangen May 4, 2023 17:16
@yannbf yannbf force-pushed the feat/turn-package-manager-to-async branch 2 times, most recently from 6ed4027 to 8d6118a Compare May 5, 2023 09:26
- This will allow us to get better flexibility in any place we run commands
@yannbf yannbf force-pushed the feat/turn-package-manager-to-async branch from 8d6118a to 46d5110 Compare May 5, 2023 10:34
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM. The only questionable bit for me was changing the function signature for executeCommand from args to an options object. I could also imagine making executeCommand sync to make it backwards compatible and introducing an executeCommandAsync with the new behavior. But I consider this an internal API so the current changes seem good.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also achieve the fix in the PR without introducing any async right? So this PR is making a bunch of changes in the name of future work that we're not actually doing right now? In which case, I'd consider just doing the bare minimum instead.

@yannbf
Copy link
Member Author

yannbf commented May 5, 2023

We could also achieve the fix in the PR without introducing any async right? So this PR is making a bunch of changes in the name of future work that we're not actually doing right now? In which case, I'd consider just doing the bare minimum instead.

This change was a request from @ndelangen as part of another PR which I'm working on. The other PR will change the way the executeCommand method behaves so that when running e.g. yarn install, no output is shown to the user, but rather it will be streamed to a log file, which will only be shown if there is an error.
To create a stream, I need to use createWriteStream, which is usable only after it emits an open event, making me need to handle it in an asynchronous nature.

In order to make that possible, there are two ways:

  • Keep things non-async with no further changes needed, but introducing bad side effects and a possible race condition
  • Changing things to async, and having no issues

Because making stuff async would make the first work generate an incredibly big PR (as you can see from this one), I decided to split into two PRs instead, so this one would be just a refactor rather than introduce bigger changes.

@shilman
Copy link
Member

shilman commented May 5, 2023

Gotcha. thanks for the clarification! 🙏

@yannbf yannbf added the patch:yes Bugfix & documentation PR that need to be picked to main branch label May 8, 2023
@yannbf yannbf merged commit 7999de3 into next May 8, 2023
59 of 62 checks passed
@yannbf yannbf deleted the feat/turn-package-manager-to-async branch May 8, 2023 14:47
shilman pushed a commit that referenced this pull request May 9, 2023
…to-async

CLI: Refactor package manager methods to be async
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label May 9, 2023
@shilman shilman mentioned this pull request May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli maintenance User-facing maintenance tasks patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants