Recover: implement callback proposal #266
Conversation
regex failed for connectionKeys with exclamation marks, negative
connection serials, etc. Client lib shouldn't assume anything about the
format of the connection key except that it doesn't contain a colon, so
a simple split(':') is safest
You can't use the same recoveryKey more than once, so unpersist after a successful recovery. Also stop persisting on connect: should only be useful in browsers that don't support beforeunload, but that's a subset of browsers that don't support sessionstorage, so not much point
as ably-js tests emit beforeunload events to test on-page-refresh functionality
|
Also, just noticed that persistence was set to last for only 15s ( |
|
|
||
| var recoverFn = self.options.recover, | ||
| lastSessionData = getFromSession && getFromSession(sessionRecoveryName); | ||
| if(lastSessionData && lastSessionData.host === window.location.host && typeof(recoverFn) === 'function') { |
There was a problem hiding this comment.
Why do we need to check lastSessionData.host == window.location.host, I would have thought the security model in the browser would look after that for us?
There was a problem hiding this comment.
Er, yep, you're completely right. Not sure why I was thinking. 😣
Because it was only intended to cover the time for the refresh to happen; not the time for the connection in the refreshed page to succeed. However, if there is a loss of connectivity it's equally possible that the page refresh stalls so perhaps it could be the same as the connection ttl. |
| var eraseCookie = (typeof(Cookie) !== 'undefined' && Cookie.erase); | ||
| var connectionKeyCookie = 'ably-connection-key'; | ||
| var connectionSerialCookie = 'ably-connection-serial'; | ||
| var getFromSession = (typeof(SessionStorage) !== 'undefined' && SessionStorage.get); |
There was a problem hiding this comment.
I find this approach a little odd. Why are we creating a SessionStorage object that sometimes has an interface with get, set and remove, and other times without the method. I would prefer we either:
- Have a method that on SessionStorage such as
isSupported()but the interface remains the same. - My preference: If
sessionStorageis not supported, we should havenoopmethods in their place that simply don't do anything.
There was a problem hiding this comment.
Sure, happy to do that. Though will still need to do (typeof(SessionStorage) !== 'undefined' && ... because in node, the sessionstorage.js file isn't even loaded, so the SessionStorage object doesn't exist
There was a problem hiding this comment.
Why not just load it and have noop methods like unsupported browsers?
There was a problem hiding this comment.
Actually on second thoughts I think it'd be simpler to leave it as-is. You still need the sessionStorageSupported checks because of node (eg because window.addEventListener isn't defined in node), so the change doesn't really let you simplify things much, sadly. 285ca82
There was a problem hiding this comment.
Why not just load it and have noop methods like unsupported browsers?
Personally I would prefer to test for the existence of the interface, and use it if present.
There was a problem hiding this comment.
@mattheworiordan sorry, github hadn't loaded your comment when I posted my last one.
The library uses a pattern of browser-specific helpers in browser/lib and node ones in node/lib (and each only loading those) for a bunch of things. I guess I wouldn't be against just having both load all files and using tests & noops if you'd prefer that (as long as you can convince Paddy), but doing that just for SessionStorage would make that inconsistent with the other browser-specific/node-specific libs, and doing it for everything would be a major refactor at this stage. So either way, I'm inclined to leave it as-is for now
|
Nice work, looks great. 👍 |
Trailing comma is invalid in IE8
|
Closing in favour of #269 |
|
Merged as 4c2c0d8 |
Fixes https://github.com/ably/realtime/issues/541
Replaces #264