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

fix: add eventEmitter on libraryCommand #5768

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

angelamuliu
Copy link

@angelamuliu angelamuliu commented Aug 27, 2024

What does this PR do?

Adds event emitter "onLibraryCommandCompletion" (accessible via CommandEventDispatcher) that fires a boolean regarding whether a library command succeeded or failed.

What issues does this PR fix or reference?

@W-16420054@

Functionality Before

Before there was no way of knowing if a library command (such as deploy) succeeded or failed.

Functionality After

Consumer can subscribe to onLibraryCommandCompletion event and understand whether command succeeded or failed.

@angelamuliu angelamuliu marked this pull request as ready for review August 28, 2024 22:26
@angelamuliu angelamuliu requested a review from a team as a code owner August 28, 2024 22:26
@peternhale peternhale removed the request for review from klewis-sfdc August 29, 2024 12:40
@peternhale peternhale changed the title chore: add eventEmitter on libraryCommand fix: add eventEmitter on libraryCommand Aug 29, 2024
@@ -220,13 +224,15 @@ export abstract class LibraryCommandletExecutor<T>
properties,
measurements
);
LibraryCommandletExecutor.libraryCommandCompletionEventEmitter.fire(!!success);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it is meaningful. LibraryCommandletExecutor is an abstract class to be extended by other real executors such as orgLogoutDefault or orgLoginAccessToken. How does the subscriber know the result from which command it is listening to with only a boolean value? Can you help me understand that? @angelamuliu @peternhale

Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch @mingxuanzhangsfdx, there is no way to know. If two commands are run concurrently and are listening for the completion event, knowing which event belongs any given listener is indeterminant.

@@ -220,13 +224,15 @@ export abstract class LibraryCommandletExecutor<T>
properties,
measurements
);
LibraryCommandletExecutor.libraryCommandCompletionEventEmitter.fire(!!success);
Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch @mingxuanzhangsfdx, there is no way to know. If two commands are run concurrently and are listening for the completion event, knowing which event belongs any given listener is indeterminant.

@peternhale
Copy link
Contributor

@angelamuliu This PR cannot move forward due to concurrency issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants