-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Conversation
6ed4027
to
8d6118a
Compare
- This will allow us to get better flexibility in any place we run commands
8d6118a
to
46d5110
Compare
There was a problem hiding this 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.
There was a problem hiding this 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.
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 In order to make that possible, there are two ways:
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. |
Gotcha. thanks for the clarification! 🙏 |
…to-async CLI: Refactor package manager methods to be async
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
Checklist
MIGRATION.MD
Maintainers
make sure to add the
ci:merged
orci:daily
GH label to it.["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]