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.

Sign up for GitHub

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
Merged

Detach event handlers after action is done #2703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/common
Open in desktop
Submodule common updated 3 files
+11 −0 helpers.ts
+12 −16 logger.ts
+119 −0 test/unit-tests/logger.ts
15 changes: 10 additions & 5 deletions lib/services/android-project-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as constants from "../constants";
import * as semver from "semver";
import * as projectServiceBaseLib from "./platform-project-service-base";
import { DeviceAndroidDebugBridge } from "../common/mobile/android/device-android-debug-bridge";
import { attachAwaitDetach } from "../common/helpers";
import { EOL } from "os";
import { Configurations } from "../common/constants";
import { SpawnOptions } from "child_process";
Expand Down Expand Up @@ -257,13 +258,17 @@ export class AndroidProjectService extends projectServiceBaseLib.PlatformProject

buildOptions.unshift("buildapk");

this.$childProcess.on(constants.BUILD_OUTPUT_EVENT_NAME, (data: any) => {
const handler = (data: any) => {
this.emit(constants.BUILD_OUTPUT_EVENT_NAME, data);
});
};

await this.executeGradleCommand(this.getPlatformData(projectData).projectRoot, buildOptions,
{ stdio: buildConfig.buildOutputStdio || "inherit" },
{ emitOptions: { eventName: constants.BUILD_OUTPUT_EVENT_NAME }, throwError: true });
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.

buildOptions,
{ stdio: buildConfig.buildOutputStdio || "inherit" },
{ emitOptions: { eventName: constants.BUILD_OUTPUT_EVENT_NAME }, throwError: true }));
} else {
this.$errors.failWithoutHelp("Cannot complete build because this project is ANT-based." + EOL +
"Run `tns platform remove android && tns platform add android` to switch to Gradle and try again.");
Expand Down
19 changes: 13 additions & 6 deletions lib/services/ios-project-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import * as semver from "semver";
import * as xcode from "xcode";
import * as constants from "../constants";
import * as helpers from "../common/helpers";
import { attachAwaitDetach } from "../common/helpers";
import * as projectServiceBaseLib from "./platform-project-service-base";
import { PlistSession } from "plist-merge-patch";
import { EOL } from "os";
Expand Down Expand Up @@ -180,7 +181,7 @@ export class IOSProjectService extends projectServiceBaseLib.PlatformProjectServ
let projectRoot = this.getPlatformData(projectData).projectRoot;
let archivePath = options && options.archivePath ? path.resolve(options.archivePath) : path.join(projectRoot, "/build/archive/", projectData.projectName + ".xcarchive");
let args = ["archive", "-archivePath", archivePath, "-configuration",
(!buildConfig || buildConfig.release) ? "Release" : "Debug" ]
(!buildConfig || buildConfig.release) ? "Release" : "Debug"]
.concat(this.xcbuildProjectArgs(projectRoot, projectData, "scheme"));
await this.$childProcess.spawnFromEvent("xcodebuild", args, "exit", { stdio: 'inherit' });
return archivePath;
Expand Down Expand Up @@ -287,15 +288,21 @@ export class IOSProjectService extends projectServiceBaseLib.PlatformProjectServ
// }
// }

this.$childProcess.on(constants.BUILD_OUTPUT_EVENT_NAME, (data: any) => {
const handler = (data: any) => {
this.emit(constants.BUILD_OUTPUT_EVENT_NAME, data);
});
};

if (buildConfig.buildForDevice) {
await this.buildForDevice(projectRoot, basicArgs, buildConfig, projectData);
await attachAwaitDetach(constants.BUILD_OUTPUT_EVENT_NAME,
this.$childProcess,
handler,
this.buildForDevice(projectRoot, basicArgs, buildConfig, projectData));
} else {
await this.buildForSimulator(projectRoot, basicArgs, projectData, buildConfig.buildOutputStdio);
await attachAwaitDetach(constants.BUILD_OUTPUT_EVENT_NAME,
this.$childProcess,
handler,
this.buildForSimulator(projectRoot, basicArgs, projectData, buildConfig.buildOutputStdio));
}

}

public async validatePlugins(projectData: IProjectData): Promise<void> {
Expand Down
10 changes: 6 additions & 4 deletions lib/services/local-build-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,23 @@
import { EventEmitter } from "events";
import { BUILD_OUTPUT_EVENT_NAME } from "../constants";
import { attachAwaitDetach } from "../common/helpers";

export class LocalBuildService extends EventEmitter {
constructor(private $projectData: IProjectData,
private $platformService: IPlatformService) {
private $platformService: IPlatformService) {
super();
}

public async build(platform: string, platformBuildOptions: IPlatformBuildData, platformTemplate?: string): Promise<string> {
this.$projectData.initializeProjectData(platformBuildOptions.projectDir);
await this.$platformService.preparePlatform(platform, platformBuildOptions, platformTemplate, this.$projectData, { provision: platformBuildOptions.provision, sdk: null });
this.$platformService.on(BUILD_OUTPUT_EVENT_NAME, (data: any) => {
const handler = (data: any) => {
data.projectDir = platformBuildOptions.projectDir;
this.emit(BUILD_OUTPUT_EVENT_NAME, data);
});
};
platformBuildOptions.buildOutputStdio = "pipe";
await this.$platformService.buildPlatform(platform, platformBuildOptions, this.$projectData);

await attachAwaitDetach(BUILD_OUTPUT_EVENT_NAME, this.$platformService, handler, this.$platformService.buildPlatform(platform, platformBuildOptions, this.$projectData));
return this.$platformService.lastOutputPath(platform, platformBuildOptions, this.$projectData);
}
}
Expand Down
10 changes: 7 additions & 3 deletions lib/services/platform-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import * as helpers from "../common/helpers";
import * as semver from "semver";
import { EventEmitter } from "events";
import { AppFilesUpdater } from "./app-files-updater";
import { attachAwaitDetach } from "../common/helpers";
import * as temp from "temp";
temp.track();
let clui = require("clui");
Expand Down Expand Up @@ -425,10 +426,13 @@ export class PlatformService extends EventEmitter implements IPlatformService {
await this.trackActionForPlatform({ action: "Build", platform, isForDevice });

let platformData = this.$platformsData.getPlatformData(platform, projectData);
platformData.platformProjectService.on(constants.BUILD_OUTPUT_EVENT_NAME, (data: any) => {
const handler = (data: any) => {
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.

};

await attachAwaitDetach(constants.BUILD_OUTPUT_EVENT_NAME, platformData.platformProjectService, handler, platformData.platformProjectService.buildProject(platformData.projectRoot, projectData, buildConfig));

let prepareInfo = this.$projectChangesService.getPrepareInfo(platform, projectData);
let buildInfoFilePath = this.getBuildOutputPath(platform, platformData, buildConfig);
let buildInfoFile = path.join(buildInfoFilePath, buildInfoFileName);
Expand Down