grbsk/ng2-idle

keepalive interval should run outside of Angular zone

Closed this issue · 7 comments

just-jeb commented

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 commented

I can issue a PR if needed.

  • 👍2
flekmatik commented

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 commented

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 commented

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

just-jeb commented

Will do in the nearest future.

michaelmills commented

Submitted a pull request: #122

grbsk commented

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.