-
-
Notifications
You must be signed in to change notification settings - Fork 195
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
fix: multiple errors in Sidekick #4888
Merged
Merged
Conversation
This file contains 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
…oper disk image when app is built in cloud on windows The livesync process is stopped when a native change is made in app built in cloud on windows OS. This happens as CLI builds the app and after that tries to restart it. As the `platformLiveSyncService.restartApplication` is without try/catch, the CLI is not able to start the application due to missing developer disk image mounted on device. After that an error is thrown and the livesync process is stopped. This PR replaces `platformLiveSyncService.restartApplication` with `this.refreshApplication`. The `this.refreshApplication` method has try/catch and shows a warning `Unable to start application...` when an error is thrown during restart process. On the other side, this method has a check if a naive change occurs, so no additional checks will be executed from the method in order to determine if the application should be refreshed or restarted. (https://github.com/NativeScript/nativescript-cli/blob/24b21c4c3ec46ecf05f3b3aa1d2ed5b7d2301424/lib/controllers/run-controller.ts#L187)
`tns run` (and using SK) should remove its handler for prepareControllers' `prepareReadyEvent` handler. However, this does not happend due to two reasons: - we try to remove the handler from runControllers' event, but this handler is attached to `preparController`, so it actually remains and causes multiple issues in long living processes, when CLI is used as a library - the event handler is not cached correctly as we attach its bound version, but cache it without the binding. So you cannot remove it. This causes issues when running on device in Sidekick, closing the current app, open it again and try to livesync a change - we have many handlers for prepareReadyEvent and this causes multiple livesync operations.
`tns preview` (and using SK) should remove its handler for prepareControllers' `prepareReadyEvent` handler. However, this does not happend due to two reasons: - we try to remove the handler from prepareControllers' event, but this handler is attached to `preparController`, so it actually remains and causes multiple issues in long living processes, when CLI is used as a library - the event handler is not cached correctly as we attach its bound version, but cache it without the binding. So you cannot remove it. This causes issues when using preview in Sidekick, closing the current app, open it again and try to livesync a change - we have many handlers for prepareReadyEvent and this causes multiple livesync operations.
Commands using prepare of project (and using SK) should remove their handlers for webpackCompilerService's `webpackCompilationComplete` event. This is not happening and whenever we start new prepartion of the project, we attach new handler. This causes issues when running on device in Sidekick, closing the current app, open it again and try to livesync a change - we have many handlers for prepareReadyEvent and this causes multiple livesync operations.
Currently the webpackCompilerService caches the started webpack processes, but it removes them from the cache only when someone calls `stopWebpackCompiler` method. This is a major problem as we cache the first instance of the child process and we do not remove it, even when the process dies. For example, when you run just build, we start the webpack process without watch mode, the webpackCompilerService caches the child process, it exits, but the instance remains cached. Trying to run the application on device does not start new webpack process and different failures occur.
`stopPreview` method does not stop running webpack compilers, so it is no longer possible to run the same application on device (no matter preview, build or run). This happens in long living processes, like Sidekick. To fix this, add new parameter to the method - an object containing the projectDir and use it to stop the processes.
rosen-vladimirov
added
bug
debug
run
Describes issues related to run command
preview
Describes issues related to preview command
labels
Jul 19, 2019
test cli-smoke cli-preview cli-run cli-templates package_version#latest |
tsvetie
approved these changes
Jul 19, 2019
test cli-smoke cli-preview cli-run cli-templates package_version#latest |
KristianDD
reviewed
Jul 19, 2019
@@ -377,7 +379,7 @@ export class RunController extends EventEmitter implements IRunController { | |||
|
|||
await this.$deviceInstallAppService.installOnDevice(device, deviceDescriptor.buildData, rebuiltInformation[platformData.platformNameLowerCase].packageFilePath); | |||
await platformLiveSyncService.syncAfterInstall(device, watchInfo); | |||
await platformLiveSyncService.restartApplication(projectData, { deviceAppData, modifiedFilesData: [], isFullSync: false, useHotModuleReload: liveSyncInfo.useHotModuleReload }); | |||
await this.refreshApplication(projectData, { deviceAppData, modifiedFilesData: [], isFullSync: false, useHotModuleReload: liveSyncInfo.useHotModuleReload }, data, deviceDescriptor); |
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.
This is the only change that bothers me a little, but refreshApplication
seems to be the only method that can handle missing DDI.
KristianDD
approved these changes
Jul 19, 2019
Fatme
approved these changes
Jul 19, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
fix(cloud, sidekick): don't stop livesync on iOS device without developer disk image when app is built in cloud on windows
The livesync process is stopped when a native change is made in app built in cloud on windows OS. This happens as CLI builds the app and after that tries to restart it. As the
platformLiveSyncService.restartApplication
is without try/catch, the CLI is not able to start the application due to missing developer disk image mounted on device. After that an error is thrown and the livesync process is stopped. This PR replacesplatformLiveSyncService.restartApplication
withthis.refreshApplication
. Thethis.refreshApplication
method has try/catch and shows a warningUnable to start application...
when an error is thrown during restart process. On the other side, this method has a check if a naive change occurs, so no additional checks will be executed from the method in order to determine if the application should be refreshed or restarted. (nativescript-cli/lib/controllers/run-controller.ts
Line 187 in 24b21c4
fix(run): remove prepareReadyEvent handler correctly on run
tns run
(and using SK) should remove its handler for prepareControllers'prepareReadyEvent
handler. However, this does not happend due to two reasons:preparController
, so it actually remains and causes multiple issues in long living processes, when CLI is used as a libraryThis causes issues when running on device in Sidekick, closing the current app, open it again and try to livesync a change - we have many handlers for prepareReadyEvent and this causes multiple livesync operations.
fix(preview): remove prepareReadyEvent handler correctly on preview
tns preview
(and using SK) should remove its handler for prepareControllers'prepareReadyEvent
handler. However, this does not happend due to two reasons:preparController
, so it actually remains and causes multiple issues in long living processes, when CLI is used as a libraryThis causes issues when using preview in Sidekick, closing the current app, open it again and try to livesync a change - we have many handlers for prepareReadyEvent and this causes multiple livesync operations.
fix: remove webpackCompilationComplete handler correctly on prepare
Commands using prepare of project (and using SK) should remove their handlers for webpackCompilerService's
webpackCompilationComplete
event. This is not happening and whenever we start new prepartion of the project, we attach new handler.This causes issues when running on device in Sidekick, closing the current app, open it again and try to livesync a change - we have many handlers for prepareReadyEvent and this causes multiple livesync operations.
fix: webpackCompilerService should not cache childProcesses forever
Currently the webpackCompilerService caches the started webpack processes, but it removes them from the cache only when someone calls
stopWebpackCompiler
method.This is a major problem as we cache the first instance of the child process and we do not remove it, even when the process dies. For example, when you run just build, we start the webpack process without watch mode, the webpackCompilerService caches the child process, it exits, but the instance remains cached.
Trying to run the application on device does not start new webpack process and different failures occur.
fix: stopPreview does not stop running webpack compilers …
stopPreview
method does not stop running webpack compilers, so it is no longer possible to run the same application on device (no matter preview, build or run). This happens in long living processes, like Sidekick.To fix this, add new parameter to the method - an object containing the projectDir and use it to stop the processes.
PR Checklist
What is the current behavior?
What is the new behavior?
Fixes/Implements/Closes #[Issue Number].