Fix simulator started on run with fake device id. #993
Conversation
|
💔 |
KristianDD
force-pushed
the
kddimitrov/fix-ios-run-fake-device
branch
from
9b45be3 to
4042ab0
Compare
|
💔 |
KristianDD
force-pushed
the
kddimitrov/fix-ios-run-fake-device
branch
from
4042ab0 to
b1abe55
Compare
|
💔 |
| try { | ||
| await this._startEmulatorIfNecessary(data); | ||
| } catch (err) { | ||
| const errorMessage = this.getEmulatorError(err, data.platform); |
There was a problem hiding this comment.
Please fix the method description, because you changed the method behavior. (now it fails without help)
There was a problem hiding this comment.
Please help me find the place where to update the description. I can't see where inside the method description is included the way it fails. There are a few places inside the method that also fail without help (that`s the reason I made it fail without help here also).
There was a problem hiding this comment.
failWithoutHelp means that command's help will not be shown, which is fine at the moment.
There was a problem hiding this comment.
You're right, just describe as best you can what the method does so it's easy to understand.
There was a problem hiding this comment.
I'm not a big fan of getting 10 rows of description what the method does. The name and the code should talk for itself. Some basic info should be enough. But describing that in case A the situation 1 will happen, and in case B situation 2 will happen is not correct, IMO. In case you are interested in all cases - you can always check the implementation.
| } | ||
|
|
||
| private async _startEmulatorIfNecessary(data?: Mobile.IDevicesServicesInitializationOptions): Promise<void> { | ||
| let deviceInstances = this.getDeviceInstances(); |
There was a problem hiding this comment.
deviceInstances is already called once on line 393, so there's no need to call it again here.
There was a problem hiding this comment.
I'd prefer that instead of passing it as an argument, but if you insist I can change it...
There was a problem hiding this comment.
Please do, because it's easier to read and there's no duplicate code.
There was a problem hiding this comment.
I'm also a fan of getting the deviceInstances here, I see no benefit of additional args of the method in this case.
Just make it const instaed of let
| @@ -672,6 +686,17 @@ export class DevicesService extends EventEmitter implements Mobile.IDevicesServi | |||
| shouldReturnImmediateResult = shouldReturnImmediateResult || false; | |||
| return { shouldReturnImmediateResult: shouldReturnImmediateResult, platform: platform }; | |||
| } | |||
|
|
|||
| private getEmulatorError(error: Error, platform: string): string { | |||
| let emulatorName = "Emulator"; | |||
There was a problem hiding this comment.
please use constants.ts / DeviceTypes and add "Simulator" there
|
|
||
| return `Cannot find connected devices.${EOL}` + | ||
| `${emulatorName} start failed with: ${error.message}${EOL}` + | ||
| `To list currently connected devices and verify that the specified identifier exists, run '${this.$staticConfig.CLIENT_NAME.toLowerCase()} device'.`; |
There was a problem hiding this comment.
Suggestion: tns device only provides the user with the information about the running devices, but the --device also accepts emulator image names. By that logic we can either tell the user to run tns device android --available-devices or make the tns device command to show the available emulator images.
There was a problem hiding this comment.
I think it will be good to add tns devices --available-devices as it is not visible inside specific platform. Moreover as tns devices lists active devices for both platform, and tns devices ios for specific platform it seems consistent that both should accept --available-devices. @rosen-vladimirov what do you think?
There was a problem hiding this comment.
I agree with the change, but you can implement it in another PR :)
There was a problem hiding this comment.
Created an issue/feature request NativeScript/nativescript-cli#3023
| try { | ||
| await this._startEmulatorIfNecessary(data); | ||
| } catch (err) { | ||
| const errorMessage = this.getEmulatorError(err, data.platform); |
There was a problem hiding this comment.
failWithoutHelp means that command's help will not be shown, which is fine at the moment.
| } | ||
|
|
||
| private async _startEmulatorIfNecessary(data?: Mobile.IDevicesServicesInitializationOptions): Promise<void> { | ||
| let deviceInstances = this.getDeviceInstances(); |
There was a problem hiding this comment.
I'm also a fan of getting the deviceInstances here, I see no benefit of additional args of the method in this case.
Just make it const instaed of let
KristianDD
force-pushed
the
kddimitrov/fix-ios-run-fake-device
branch
from
c312324 to
5ff2794
Compare
Plamen5kov
force-pushed
the
kddimitrov/fix-ios-run-fake-device
branch
from
5ff2794 to
1533d41
Compare
This fixes NativeScript/nativescript-cli#2728. Before a simulator with default preference was started when using
tns run ios --device "fake"and nothing else happens. Now a descriptive error is show to the user.SHOULD NOT BE MERGED BEFORE - NativeScript/ios-sim-portable#92