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

feat: prevent timing out one tab or window if another tab have activity #33

Merged
merged 4 commits into from
Feb 9, 2017
Merged

feat: prevent timing out one tab or window if another tab have activity #33

merged 4 commits into from
Feb 9, 2017

Conversation

roupify
Copy link
Contributor

@roupify roupify commented Feb 7, 2017

Add a LocalStorageExpiry that put the expiry value in LocalStorage. If localStorage is not supported
by the browser, will store the expiry value in memory.

Please check if the PR fulfills these requirements

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

[ ] Bugfix
[X] 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)

Multiple tabs or windows wasn't supported.

What is the new behavior?

Now, the expiry value is set in the localStorage by default, and if a browser doesn't support it (i.e. Safari in private mode), we store the expiry value in memory.

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:

dd a LocalStorageExpiry that put the expiry value in LocalStorage. If localStorage is not supported
by the browser, will store the expiry value in memory.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 98.267% when pulling 2be6a45 on rousseaufiliong:localStorage into a682826 on HackedByChinese:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 98.762% when pulling 50d30c4 on rousseaufiliong:localStorage into a682826 on HackedByChinese:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 99.01% when pulling 1f8b29c on rousseaufiliong:localStorage into a682826 on HackedByChinese:master.

Copy link
Owner

@grbsk grbsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing I learned the hard way from ng-idle was not to be too prescriptive or bundle in anything that wasn't absolutely essential, thus why this rewrite is very modular (perhaps irritatingly so).

Thus, rather than making this the new default and including in /core, should we make it a new module ala keepalive and let the dev decide if they want this? I'm kind of on the fence about this myself, as it's probable that most people would want this rather than SimpleExpiry anyways.

}

private getExpiry(): Date {
let expiry: string = this.localStorage.getItem('expiry');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing people have been asking for from ng-idle was the ability to change the key so that if they had multiple Angular apps in one domain (or, as a new challenge now that we can support multiple Idle instances per app) they don't conflict.

We could allow for this by taking in a new constructor parameter key: string (defaulting to the current expiry) and use that when getting or setting localStorage. The developer could then opt-in to this feature by initializing and providing a new instance rather than the current default LocalStorageExpiry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea, I'll do the changes this morning.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 99.279% when pulling cfd0b15 on rousseaufiliong:localStorage into a682826 on HackedByChinese:master.

@grbsk grbsk merged commit 3ab086d into grbsk:master Feb 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants