Skip to content

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

Merged
merged 1 commit into from
Apr 11, 2017

Conversation

Mitko-Kerezov
Copy link
Contributor

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

@Mitko-Kerezov Mitko-Kerezov self-assigned this Apr 7, 2017
@Mitko-Kerezov Mitko-Kerezov force-pushed the kerezov/multiple-event-handlers branch 2 times, most recently from 6f8a6d6 to 9839a2b Compare April 10, 2017 14:09
await attachAwaitDetach(constants.BUILD_OUTPUT_EVENT_NAME,
this.$childProcess,
handler,
this.executeGradleCommand(this.getPlatformData(projectData).projectRoot,
Copy link
Contributor

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.

Copy link
Contributor Author

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());
Copy link
Contributor

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?

Copy link
Contributor Author

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:

  1. You spawn from Fusion(tns is require-d) - then you spawn with pipe 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
  2. You spawn from CLI (tns executed from command line) - you then spawn with inherit, this event handler will not execute because all of the output is printed on process.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.

@Mitko-Kerezov Mitko-Kerezov force-pushed the kerezov/multiple-event-handlers branch from 9839a2b to c8dcac0 Compare April 11, 2017 07:40
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.
@Mitko-Kerezov Mitko-Kerezov force-pushed the kerezov/multiple-event-handlers branch from c8dcac0 to 018ca1e Compare April 11, 2017 08:00
@Mitko-Kerezov Mitko-Kerezov merged commit 1606b2a into master Apr 11, 2017
@Mitko-Kerezov Mitko-Kerezov deleted the kerezov/multiple-event-handlers branch April 11, 2017 09:04
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