Remove ios native dependencies#2495
Merged
TsvetanMilanov merged 8 commits intomasterfrom Mar 14, 2017
Merged
Conversation
rosen-vladimirov
suggested changes
Feb 8, 2017
Contributor
rosen-vladimirov
left a comment
rosen-vladimirov
left a comment
There was a problem hiding this comment.
The title says: Remove ios native dependencies
However the PR just adds dependency in package.json. Shouldn't we actually delete the native dependencies from package.json?
TsvetanMilanov
force-pushed
the
milanov/remove-ios-native-dependencies
branch
3 times, most recently
from
4c05959 to
8893b67
Compare
TsvetanMilanov
force-pushed
the
milanov/remove-ios-native-dependencies
branch
from
8893b67 to
cfee565
Compare
rosen-vladimirov
suggested changes
Feb 26, 2017
|
|
||
| npc.postNotificationAndAttachForData(this.$iOSNotification.attachRequest); | ||
| }); | ||
| // We should create these promises here beecause we need to send the ObserveNotification on the device |
Contributor
There was a problem hiding this comment.
beecause -> because
| private async executeAttachAvailable(deviceIdentifier: string, timeout: number): Promise<void> { | ||
| try { | ||
| await this.$iOSNotificationService.awaitNotification(npc, this.$iOSNotification.readyForAttach, timeout); | ||
| // We should create this promise here beecause we need to send the ObserveNotification on the device |
| await this.$iOSNotificationService.postNotificationAndAttachForData(deviceIdentifier, this.$iOSNotification.attachRequest); | ||
| await Promise.all(promisesToWait); | ||
| } catch (e) { | ||
| this.$logger.trace(`Timeout error: ${e}`); |
Contributor
There was a problem hiding this comment.
Is this really a timeout error now? Is it possible something else to fail?
|
|
||
| await this.$iOSNotificationService.awaitNotification(npc, this.$iOSNotification.readyForAttach, readyForAttachTimeout); | ||
| await this.$iOSNotificationService.postNotificationAndAttachForData(deviceIdentifier, this.$iOSNotification.attachRequest); | ||
| await Promise.all(promisesToWait); |
Contributor
There was a problem hiding this comment.
hm, is Promise.all the correct method here. The order before was:
- appLaunching
- readyForAttach
- waitForDebug (optional)
- attachRequest
Now, with current code, it is:
- appLaunching, readyForAttach (simultaneously)
- waitForDebug
- attachRequest
Or even in some cases:
- waitForDebug
- attachRequest
- appLaunching, readyForAttach (simultaneously)
Can you elaborate why is the change of the order?
TsvetanMilanov
force-pushed
the
milanov/remove-ios-native-dependencies
branch
5 times, most recently
from
a0bed5e to
12b0d08
Compare
We need to remove our native dependencies - ref and ffi because they are casuing problems with each version of NodeJS which has new version of V8 in it. We are using ref and ffi to communicate with iTunes and execute fs and application management operations on iOS devices. This operations are moved in new dependency - device-lib. The device lib has JavaScript wrapper which we use in the CLI. Everything in the ios device file system and ios device application manager is replaced with the exposed methods from the device lib.
Mitko-Kerezov
force-pushed
the
milanov/remove-ios-native-dependencies
branch
from
d0b8154 to
3eb893a
Compare
TsvetanMilanov
force-pushed
the
milanov/remove-ios-native-dependencies
branch
2 times, most recently
from
4ebcb94 to
a170d90
Compare
Since we use the ios-device-lib to connect to the device backend port for debugging, the socket returned from the device can be used only in the ios-device lib. That's why we need to change the logic for creating the Node.js socket in the CLI. The ios-device-lib will expose the device socket on a specific port in the localhost. After we connect to the exposed socket we can continue to work like we used to.
TsvetanMilanov
force-pushed
the
milanov/remove-ios-native-dependencies
branch
from
a170d90 to
c9d03b1
Compare
rosen-vladimirov
approved these changes
Mar 13, 2017
TsvetanMilanov
force-pushed
the
milanov/remove-ios-native-dependencies
branch
from
84ed8ae to
0280bd9
Compare
TsvetanMilanov
force-pushed
the
milanov/remove-ios-native-dependencies
branch
from
0280bd9 to
9e30d2b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We need to remove our native dependencies - ref and ffi because they are casuing problems with each version of NodeJS which has new version of V8 in it. We are using ref and ffi to communicate with iTunes and execute fs and application management operations on iOS devices. This operations are moved in new dependency - device-lib. The device lib has JavaScript wrapper which we use in the CLI. Everything in the ios device file system and ios device application manager is replaced with the exposed methods from the device lib.
Common lib reference: telerik/mobile-cli-lib#889