-
-
Notifications
You must be signed in to change notification settings - Fork 197
Detach event handlers after action is done #2703
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
Conversation
6f8a6d6
to
9839a2b
Compare
await attachAwaitDetach(constants.BUILD_OUTPUT_EVENT_NAME, | ||
this.$childProcess, | ||
handler, | ||
this.executeGradleCommand(this.getPlatformData(projectData).projectRoot, |
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.
In case this method fails before returning the Promise (for example in case the implementation is:
myMethod(): Promise<any> {
// some sync operation that throws error
return new Promise<any>((resolve, reject) => {
// execute some operations
resolve();
});
}
current code will fail during evaluation of method arguments and will never emit error event for the operation. Is this expected?
Note: I know we are using async
keyword in such cases that will always generate Promise for us and we should never end up in a case where we fail before creating the Promise, but anyway, I want to be sure that's the behaviour we want.
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.
IMO it should be the caller's responsibility to keep track of the arguments. If one of the arguments need to be prepared in some way (e.g. to be try...catch
-ed) it should be done prior to calling the method.
As for the example you've provided I see no problem with it - failing during evaluation of the arguments will lead to the event handler not being attached in the first place and this method's main target is preventing leakage of event handlers.
this.emit(constants.BUILD_OUTPUT_EVENT_NAME, data); | ||
}); | ||
await platformData.platformProjectService.buildProject(platformData.projectRoot, projectData, buildConfig); | ||
this.$logger.printInfoMessageOnSameLine(data.data.toString()); |
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.
why do we have this line and we do no have it in the other cases when we use attachAwaitDetach?
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 need in order to keep the current behavior when using tns
as a CLI.
Whenever we spawn a child process with pipe
instead of inherit
as its stdio, we will not see the output on the console.
In short you have 2 cases:
- You spawn from Fusion(
tns
isrequire
-d) - then you spawn withpipe
and you need this line in order to see the output on the console and you need the event handler in order to capture output and forward it to fusion - You spawn from CLI (
tns
executed from command line) - you then spawn withinherit
, this event handler will not execute because all of the output is printed onprocess.stdout
.
Bottom line - you need this in the unlikely case that you spawn with pipe
from the terminal so that you see the build output on the console.
9839a2b
to
c8dcac0
Compare
In order to not emit `buildOutput` event multiple times during build in a long running process (fusion) we have to detach event handlers after build is done.
c8dcac0
to
018ca1e
Compare
In order to not emit
buildOutput
event multiple times during build in a long running process (fusion) we have to detach event handlers after build is done.Merge after telerik/mobile-cli-lib#940
Ping @rosen-vladimirov @TsvetanMilanov