Fix(core): [On behalf of Lexmark International, Inc]: Check expiry be…

[markmaynard]

…fore toggling idle state

Please check if the PR fulfills these requirements

• The commit message follows our guidelines: https://github.com/HackedByChinese/ng2-idle/blob/master/CONTRIBUTING.md#pr
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[X] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
Expiry is not checked for updated timeout before going idle:
#27

What is the new behavior?
Expiry is checked and if store timeout is greater than current time a new timeout is set and idle is not toggled.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[X] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

[markmaynard]

Tried to run gulp tests but didn't find any, I see now that the tests are an npm step now. I'll look at what I broke Monday and try and get the tests up to date.

[markmaynard]

I did a little more digging and it looks like this change is somehow interfering with tick(). Meaning that the call to expiry.now() is not returning a an incremented time value.

[markmaynard]

Found the reason for test issues but unsure how to solve. Angular 2 tick only causes events to fire, it does not however, override time. This means that Date.now() isn't returning a time that is offset by the tick calls. My code is functioning correctly, but the tests fail due to tick not actually altering time.

[coveralls]

Coverage decreased (-0.5%) to 98.815% when pulling 68889af on markmaynard:master into 708aa9f on HackedByChinese:master.

[coveralls]

Coverage increased (+0.01%) to 99.289% when pulling 9f8c893 on markmaynard:master into 708aa9f on HackedByChinese:master.

[coveralls]

Coverage increased (+0.01%) to 99.289% when pulling dbfb144 on markmaynard:master into 708aa9f on HackedByChinese:master.

[grbsk]

I see the approach you have, but I'm still thinking the best solution is to just add a new custom InterruptSource that checks the value of the cookie on a timer, and if the value is more recent than the last time you checked it, trigger the onInterrupt event (giving it an argument that has force: true). This will cause idleness to be interrupted closer to real time, whereas this PR won't update anything until the idle timeout expires. I worry this lag could cause some out-of-sync weirdness between two tabs. I'd also like to avoid having expiry detection in multiple places (interrupt() and watch()).

So sort of like with the LocalStorage functionality that was added, you can keep your CookieExpiry and then just add CookieInterruptSource with the cookie checking timer. The way I devised it is that IdleExpiry (and its derivatives) handle storing expiry and checking to see if the value has indeed expired, and InterruptSource (and its derivatives) is responsible for monitoring for or synthesizing an interrupt based on some sort of activity (which can be a browser event, a change to local storage, or a change to a cookie value). The way this PR works is we've kind of hacked in a combination of expiry and interrupt cleanup logic into watch() which is was not intended to do. This could create side effects for other people trying to extend ng-idle in the future.

Does that make sense? I can help you with a cookie interrupt source if need be.

[grbsk]

Hell we can even create a new optional module called @ng-idle/cookies that includes your CookieExpiry and a new CookieInterruptSource.

[markmaynard]

The two problems I was trying to avoid were an infinite loop, and excessive polling.

inifinite loop - I was worried that if I made the change to add the cookieExpiry triggering an interval that would cause watch to set the cookie again, which would set the cookie and cause an infinite loop. If you use force to skip setExpiry to prevent the loop you run into another problem. You have now extended the idle time by the full amount instead of the delta between idle and what was set in the cookie, this may not be a BIG issue but I still think it's incorrect.

excessive polling - We know that we don't need to check idle again for at least what we set the interval to initially, so we shouldn't need to poll over-and over again every second.

Thanks for your response, do you think your suggested approach tackles these issues?

[stevefarwell]

Any plans to add CookieExpiry and a new CookieInterruptSource? This would be great for managing across tabs and sub-domains

[markmaynard]

@stevefarwell I have that code(cookie-expiry) for the sub-domain problem and am willing to PR(pending corporate approval) if it is desired by the project owner.

[stevefarwell]

@markmaynard let me know if I can help

[stevefarwell]

@markmaynard any chance on getting your cookie-expiry code?

[sharathreddyt]

@markmaynard is there any chance on getting your cookie-expiry code?

[markmaynard]

@sharathreddyt I'll try and work on it.

[markmaynard]

@sharathreddyt @stevefarwell here is a gist of the expiry code for cookies:
https://gist.github.com/markmaynard/65bb8708dd615d149d89c29111e963c2