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

Add onPageRefresh option #264

Closed
wants to merge 4 commits into from
Closed

Add onPageRefresh option #264

wants to merge 4 commits into from

Conversation

SimonWoolf
Copy link
Member

@SimonWoolf SimonWoolf commented Apr 29, 2016

326e78d Fixes #259.

Currently this leaves none as the default, same behaviour as now.

Also includes c62fc42 as a proposed solution to https://github.com/ably/realtime/issues/540: turn off persisting the connection onConnect, so that it's only persisted on page refresh; and unpersist after a successful recovery. Former change minimises the time the cookie is persisted to the duration of the refresh/reconnect cycle, rather than the whole time. Latter change prevents a recovery key from being used twice (unless there's a race condition between tabs, which is much less likely). The downside is we lose the fallback recover-from-key-only (no serial) functionality - but afaict that's really only useful if onPageRefresh isn't supported, which at this point is very few browsers (safari < 3 and that's about it), so I don't think we're losing much.

We can minimise interference between tabs by only persisting the
connection when the page refreshes, and then unpersisting it once we
connect with the recovered key/serial. (At the cost of losing the
fallback recover-from-key-only if the onPageRefresh callback fails)
as ably-js tests emit beforeunload events to test on-page-refresh
functionality
@SimonWoolf
Copy link
Member Author

Closing in favour of #266

@SimonWoolf SimonWoolf closed this May 5, 2016
@SimonWoolf SimonWoolf deleted the onpagerefresh branch May 20, 2016 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant