Skip to content

fix(NativeScriptPlatformRef): Destroy lastModuleRef on exitEvent #1728

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

Merged
merged 14 commits into from
Apr 15, 2019
Merged

fix(NativeScriptPlatformRef): Destroy lastModuleRef on exitEvent #1728

merged 14 commits into from
Apr 15, 2019

Conversation

m-abs
Copy link
Contributor

@m-abs m-abs commented Feb 12, 2019

What is the current behavior?

The application module isn't destroyed when the application exitEvent is triggered.
This causes the previous instance of the nativescript-angular app to be running in a sleep state and ngOnDestroy() on it's compoments, services etc. are not called as expected.

What is the new behavior?

Adds an event listener for the exitEvent in the NativeScriptPlatformRef which destroys the lastModuleRef which triggers ngOnDestroy() as expected.

Fixes #923

@ghost ghost added the ♥ community PR label Feb 12, 2019
@vakrilov
Copy link
Contributor

test

vakrilov
vakrilov previously approved these changes Feb 14, 2019
@vakrilov
Copy link
Contributor

vakrilov commented Feb 14, 2019

LGTM - triggered the CI.
Also I think this is more of a fix than a chore so I have renamed the PR title.

@vakrilov vakrilov changed the title chor(NativeScriptPlatformRef): Destroy lastModuleRef on exitEvent fix(NativeScriptPlatformRef): Destroy lastModuleRef on exitEvent Feb 14, 2019
@ghost ghost assigned vakrilov Feb 19, 2019
@ghost ghost added in progress and removed ♥ community PR labels Feb 19, 2019
@vakrilov
Copy link
Contributor

test

vakrilov
vakrilov previously approved these changes Feb 21, 2019
@etabakov
Copy link
Contributor

@cla-bot check

@cla-bot cla-bot bot added the cla: yes label Feb 25, 2019
@cla-bot
Copy link

cla-bot bot commented Feb 25, 2019

The cla-bot has been summoned, and re-checked this pull request!

@m-abs
Copy link
Contributor Author

m-abs commented Mar 8, 2019

Hi @vakrilov,

I just noticed a problem

On Android 9.

  1. Launch an app with this patch applied.
  2. Go to system settings -> Accessibility -> Font size and change the font size.
  3. Go back to the app.

This will have triggered an exit event and tried to destroy the angular application, but it is still running.
Now in a broken state.

You can see the problem here:

  1. Checkout https://github.com/m-abs/tns-ng-leak
  2. npm install && bash ./patches/apply-patches.sh
  3. Follow the steps above.

I print out the app events:

JS: suspend # Leave the app to change the font size
JS: activityPaused
JS: activityStopped
JS: orientationChanged # Font size changed in while I'm in settings
JS: exit # Returned to the app
JS: activityDestroyed
JS: activityCreated
JS: activityStarted
JS: activityResumed
JS: resume
JS: displayed
JS: displayed

m-abs added 3 commits March 8, 2019 15:40
If exit event was triggered twice we would try to destroy an already
destroy module ref.
Don't destroy the angular module on restart
@m-abs
Copy link
Contributor Author

m-abs commented Mar 11, 2019

I solved the problem.

The exit event is triggered because the MainActivity.onDestroy() is called as a part of a restart needed to apply the new settings.

I added a test for this case in the exit event handler.

@ghost ghost assigned elena-p Apr 12, 2019
@SvetoslavTsenov
Copy link
Contributor

test

@elena-p elena-p merged commit f12000e into NativeScript:master Apr 15, 2019
@ghost ghost removed the in progress label Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ngOnDestroy not called on Android back button
5 participants