Skip to content

feat: reduce HMR crashes in Preview and recover properly after them #5098

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

Jump to bottom
Merged
merged 4 commits into from
Nov 21, 2019
Merged

feat: reduce HMR crashes in Preview and recover properly after them #5098

merged 4 commits into from
Nov 21, 2019

Conversation

DimitarTachev
Copy link
Contributor

PR Checklist

What is the current behavior?

  1. We are not able to recover after an HMR crash on iOS. (it's working on Android just because the Preview app is requesting new files after crashes).
  2. There's no indication when we've lost connection with the devices. The CLI is just not printing anything.
  3. We are not printing any hints for starting the Preview app when there is an HMR status timeout.
  4. The device-specific logs are misleading as they are not printed with a device name prefix.
  5. Run with HMR is not assuming the HMR status timeout as HMR error and not retrying with full sync.
  6. HMR updates are not chained causing wrong statuses and crashes from parallel updates.
  7. HMR updates are not batched on fast changes leading to slow app updates.

What is the new behavior?

  1. We are executing HMR or full sync based on the previous HMR sync status.
  2. "No connected devices" info is logged when there aren't any connected devices.
  3. A hint for starting the Preview app is logged on HMR status timeout.
  4. The device-specific logs are printed with a device name prefix.
  5. Run with HMR is now assuming the HMR status timeout as HMR error and retrying with full sync.
  6. HMR updates are now chained in order to avoid getting a wrong status from parallel updates.
  7. HMR updates are batched in order to avoid slower updates on fast changes.

The image below shows three possible solutions to the above-mentioned problems:
Preview LiveSync
After analyzing the CLI logs, we found out that just a few users are using tns preview with more than one device from the same platform and this PR is implementing the simplest solution (the simplest but more expensive path from the diagram). In other words, it won't generate much more traffic through the message queue as almost all of the users are using just a single device per platform.

Related to: NativeScript/playground-feedback#139

@cla-bot cla-bot bot added the cla: yes label Oct 25, 2019
@DimitarTachev DimitarTachev force-pushed the tachev/preview-recovery branch from 8b89dee to 107dadb Compare October 25, 2019 12:08
@DimitarTachev DimitarTachev added the e2e test needed Describes that issue requires functional test label Oct 25, 2019
@DimitarTachev DimitarTachev force-pushed the tachev/preview-recovery branch from 107dadb to b4b8e62 Compare October 28, 2019 07:45
@DimitarTachev DimitarTachev mentioned this pull request Oct 28, 2019
Open
@DimitarTachev DimitarTachev force-pushed the tachev/preview-recovery branch from b4b8e62 to 8d44bc9 Compare October 28, 2019 09:45
@DimitarTachev
Copy link
Contributor Author

test cli-preview cli-run cli-templates

@DimitarTachev
feat: recover after HMR crashes in Preview
6bd024a 
Additional Details and Improvements:
- no connected devices info is logged when there aren't any connected devices
- a hint for starting the Preview app is logged on HMR status timeout
- the device specific logs are printed with a device id prefix
- run with HMR is now assuming the Status timeout as HMR error and retrying with a full sync
- HMR status reading is fixed - we are attaching to the proper Preview logs provider as the previewSdkService is not emitting any logs
- HMR updates are now chained in order to avoid getting wrong status from parallel updates
- HMR updates are batched in order to avoid slower updates on fast changes
@DimitarTachev DimitarTachev force-pushed the tachev/preview-recovery branch from 8d44bc9 to 6bd024a Compare October 31, 2019 11:57
@miroslavaivanova
Copy link
Contributor

test

@miroslavaivanova
Copy link
Contributor

When you use the command: tns preview --no-hmr, livesync doesn't work.

@DimitarTachev
Copy link
Contributor Author

test cli-preview cli-run cli-templates

@DimitarTachev
Copy link
Contributor Author

test --ignore

@miroslavaivanova miroslavaivanova merged commit e18a8a7 into master Nov 21, 2019
@miroslavaivanova miroslavaivanova mentioned this pull request Nov 21, 2019
Merged
@rosen-vladimirov rosen-vladimirov deleted the tachev/preview-recovery branch January 17, 2020 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KristianDD KristianDD KristianDD approved these changes

Assignees
No one assigned
Labels
cla: yes e2e test needed Describes that issue requires functional test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DimitarTachev @miroslavaivanova @KristianDD