Skip to content
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

keepalive interval should run outside of Angular zone #113

Closed
just-jeb opened this issue Aug 6, 2018 · 7 comments
Closed

keepalive interval should run outside of Angular zone #113

just-jeb opened this issue Aug 6, 2018 · 7 comments
Labels

Comments

@just-jeb
Copy link

just-jeb commented Aug 6, 2018

I'm submitting a ... (check one with "x")

[x ] bug report => search github for a similar issue or PR before submitting
[ ] feature request
[ ] support request => Please do not submit support request here, instead see https://github.com/HackedByChinese/ng2-idle/blob/master/CONTRIBUTING.md#getting-help

Current behavior

Keepalive interval is run inside Angular zone, which causes Testability to be unstable.

Expected behavior

Keepalive interval should not affect Testability state, i.e. run outside of Angular zone.

Minimal reproduction of the problem with instructions

  1. Set interval: keepalive.inteval(1000)
  2. Start keepalive: keepalive.start()
  3. Check the Testability state: getAllAngularTestabilities()[0].isStable()
  4. See that it is unstable, i.e. false

What is the motivation / use case for changing the behavior?

To let the e2e tests pass in applications that use keepalive.

Please tell us about your environment:

Windows 7, IntelliJ, npm, Angular dev server

  • @ng-idle version: 2.x

2.0.0-beta.15 & 6.0.0-beta.3

  • Angular version: 2.x

5.2.11 & 6.1.2

  • Browser: [all | Chrome XX | Firefox XX | IE XX | Safari XX | Mobile Chrome XX | Android X.X Web Browser | iOS XX Safari | iOS XX UIWebView | iOS XX WKWebView ]

all

  • Language: [all | TypeScript X.X | ES6/7 | ES5]
    all
  • Node: node --version =
    8.11.3

Suggested solution:
Keepalive interval function should be run outside of Angular zone, while onPing emission should run inside the Angular zone.

@just-jeb
Copy link
Author

just-jeb commented Aug 8, 2018

I can issue a PR if needed.

@flekmatik
Copy link

Do I understand correctly that no one can use protractor for testing his application once this package is used? Is there any workaround?

@just-jeb
Copy link
Author

There is actually. If you run keepalive.interval() outside of Angular zone this will work. Worked for me.
But I still think this behavior should come out of the box with this package.

@flekmatik
Copy link

Thanks!! It would be great if you made a pull request, then we can source that one directly.

@just-jeb
Copy link
Author

Will do in the nearest future.

michaelmills pushed a commit to michaelmills/ng2-idle that referenced this issue Apr 4, 2019
This is a fix for issue grbsk#113. Moved the setInterval call in keepalive.start() to run outside of
NgZone so it won't impede on Protractor testing. The setInterval callback will run inside NgZone so
that Angular change detection will still work.

issue grbsk#113
@michaelmills
Copy link

michaelmills commented Apr 4, 2019

Submitted a pull request: #122

grbsk added a commit that referenced this issue Apr 4, 2019
Fixes bug #113: Run keepalive setInterval outside NgZone
@grbsk grbsk added the bug label Jul 12, 2019
@grbsk
Copy link
Owner

grbsk commented Jul 23, 2019

Whoops, this PR was merged and I think has been in the codebase for a while now. It's definitely in the 8.0.0 beta builds.

@grbsk grbsk closed this as completed Jul 23, 2019
grbsk added a commit that referenced this issue Jul 23, 2019
…option

ssr option instructs interrupt sources to ignore targets derived from global context such as window
or document so they can be safely used in ssr/universal apps.

Fixes #113
grbsk added a commit that referenced this issue Jul 23, 2019
…option

ssr option instructs interrupt sources to ignore targets derived from global context such as window
or document so they can be safely used in ssr/universal apps.

Fixes #113
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants