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

Support optional multiple recovery scopes #1762

Merged
merged 1 commit into from
May 21, 2024
Merged

Support optional multiple recovery scopes #1762

merged 1 commit into from
May 21, 2024

Conversation

SimonWoolf
Copy link
Member

New recoveryScope client option where, if specified, the SDK's recovery key will be persisted under that identifier (in sessionstorage), so only another library instance which specifies the same recoveryScope will attempt to recover from it. This is useful if you have multiple ably-js instances sharing a given origin (the origin being the scope of sessionstorage), as otherwise the multiple instances will overwrite each other's recovery keys, and after a reload they will all try and recover the same connection, which is not permitted and will cause broken behaviour.

Not entirely happy with recoveryScope but I can't think of anything better (considered: recoveryKeyKey, recoveryKeyIdentifier, recoveryDiscriminator, recoveryTag, recoveryMarker, ...)

@SimonWoolf SimonWoolf requested a review from ttypic May 15, 2024 17:12
@github-actions github-actions bot temporarily deployed to staging/pull/1762/features May 15, 2024 17:12 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1762/bundle-report May 15, 2024 17:13 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1762/typedoc May 15, 2024 17:13 Inactive
Copy link
Collaborator

@ttypic ttypic left a comment

Choose a reason for hiding this comment

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

Agreed that scope is not perfect. To me, it implies that two clients can share the same scope, which is incorrect. What do you think about name? name should be given and unique, for example, recoveryKeyName.

Also, what do you think about making this feature more general-purpose? For example, clientName or something similar. We can reuse it for other purposes, such as labeling logs. We've received complaints before that if you use more than one client, it's impossible to work with logs.

Lastly, who will be responsible for adding this to the specification?

@SimonWoolf
Copy link
Member Author

Agreed that scope is not perfect. To me, it implies that two clients can share the same scope, which is incorrect. What do you think about name? name should be given and unique, for example, recoveryKeyName.

Maybe. At this point the words "recovery key" have started to glaze over for me so I've made a poll in #topic-engineering to gather opinions.

Lastly, who will be responsible for adding this to the specification?

Features that are specific to ably-js, like sessionstorage persistence on beforeunload, or jsonp, or the comet transport don't typically get added to the spec. (the spec is to ensure consistency of implementations, we've generally taken the view that it's not needed if there's only one implementation)

Also, what do you think about making this feature more general-purpose? For example, clientName or something similar. We can reuse it for other purposes, such as labeling logs. We've received complaints before that if you use more than one client, it's impossible to work with logs.

That is indeed a bug, but the bug is that the logger is global. So we can't add anything a client name to each logger instance, because there are no separate logger instances. Once that bug is fixed, I'm not sure a clientName would be particularly needed -- if we want to add a unique string to each log we can use the connectionId, which has the advantage that it doesn't require any customer action and we can correlate with serverside logs. Or if someone wants something fancier they can pass in a logHandler.

In particular I think we should have a bias against adding new spec'd client options (i.e. options that we want to add to the spec and all sdks) unless we're sure they're justified. They add lots of work for us (getting every sdk into compliance), and that bit more complexity to the customer for someone scanning down a list of options (not to mention the potential for confusion -- clientName vs clientId etc).

Copy link
Contributor

@VeskeR VeskeR left a comment

Choose a reason for hiding this comment

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

LGTM overall, would like to re-review once the naming is settled.

New client option where, if specified, the SDK's recovery key will be persisted under
that identifier (in sessionstorage), so only another library instance which specifies
the same recoveryScope will attempt to recover from it. This is useful if you have
multiple ably-js instances sharing a given origin (the origin being the scope of
sessionstorage), as otherwise the multiple instances will overwrite each other's
recovery keys, and after a reload they will all try and recover the same connection,
which is not permitted and will cause broken behaviour.

renamed recoveryScope to recoveryKeyStorageName
@SimonWoolf
Copy link
Member Author

based on the poll results I've gone with recoveryKeyStorageName. Not a fan myself but meh whatever

@SimonWoolf
Copy link
Member Author

@ttypic poke

Copy link
Collaborator

@ttypic ttypic left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@SimonWoolf SimonWoolf merged commit 067f4b0 into main May 21, 2024
9 of 12 checks passed
@SimonWoolf SimonWoolf deleted the recovery-scopes branch May 21, 2024 08:54
@ttypic ttypic mentioned this pull request May 29, 2024
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.

3 participants