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

AuthCallback and InstanceStore Refactoring and documentation #250

Merged
merged 25 commits into from
Dec 2, 2021

Conversation

ben-xD
Copy link
Contributor

@ben-xD ben-xD commented Nov 23, 2021

This work improves authCallback documentation so users can test locally more easily.

It also simplifies the code further, continuing the work of #231.

Mainly,

    void setRealtimeToken(long handle, Object tokenDetails) {
        _realtimeTokenData.put(handle, tokenDetails);
    }

    Object getRealtimeToken(long handle) {
        Object token = _realtimeTokenData.get(handle);
        _realtimeTokenData.remove(handle);
        return token;
    }

There is no need to save tokenDetails in AblyLibrary, it can be returned immediately in the callback.

Once there, I wanted to improve the naming of AblyLibrary. It only does one thing: store instances, and so it's been renamed to AblyInstanceStore.

I also move the logic of creating clients outside of AblyInstanceStore. This paves the way for futher improvements: moving usage of AblyInstanceStore into the Codec as opposed to the method call handlers. Therefore, in the future, we might not need to deal with "handles", since we could deserialize the handle into the instance in the codec. This helps a transition to Swift too, since things are more decoupled.

There's lots of refactoring going on in this file. There is no logic changes, except for the one above.

I'm sorry for making this PR so big.

@ben-xD ben-xD added the enhancement New feature or improved functionality. label Nov 23, 2021
@ben-xD ben-xD self-assigned this Nov 23, 2021
@github-actions github-actions bot temporarily deployed to staging/pull/250/dartdoc November 23, 2021 10:49 Inactive
… the StreamsChannel

I also rename AblyLibrary to AblyInstanceStore. It stores instances
@github-actions github-actions bot temporarily deployed to staging/pull/250/dartdoc November 23, 2021 11:08 Inactive
@ben-xD ben-xD marked this pull request as draft November 23, 2021 11:08
@ben-xD ben-xD changed the base branch from enhancement/log-level-enum to fix/ios/remove-background-queue November 23, 2021 11:29
@ben-xD ben-xD changed the title Improve authCallback documentation for local debugging and simplify AblyLibrary AuthCallback and InstanceStore Refactoring and documentation Nov 23, 2021
@github-actions github-actions bot temporarily deployed to staging/pull/250/dartdoc November 23, 2021 11:31 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/250/dartdoc November 23, 2021 11:35 Inactive
@ben-xD ben-xD requested a review from tiholic November 23, 2021 16:03
@github-actions github-actions bot temporarily deployed to staging/pull/250/dartdoc November 23, 2021 16:04 Inactive
@ben-xD ben-xD marked this pull request as ready for review November 23, 2021 16:04
@github-actions github-actions bot temporarily deployed to staging/pull/250/dartdoc November 23, 2021 17:02 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/250/dartdoc November 23, 2021 21:35 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/250/dartdoc November 29, 2021 09:38 Inactive
Base automatically changed from fix/ios/remove-background-queue to main November 29, 2021 21:42
Copy link
Contributor

@lukasz-szyszkowski lukasz-szyszkowski left a comment

Choose a reason for hiding this comment

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

Please re-check your files and add newline at the end of each modified file.

ios/Classes/AblyInstanceStore.h Show resolved Hide resolved
ios/Classes/AblyInstanceStore.m Show resolved Hide resolved
# Conflicts:
#	android/src/main/java/io/ably/flutter/plugin/AblyMethodCallHandler.java
@github-actions github-actions bot temporarily deployed to staging/pull/250/dartdoc November 30, 2021 10:32 Inactive
…auth-callback-state

# Conflicts:
#	android/src/main/java/io/ably/flutter/plugin/AblyMethodCallHandler.java
@ben-xD ben-xD force-pushed the enhancement/simplify-auth-callback-state branch from 08b63ca to 2b86b3e Compare November 30, 2021 13:03
@github-actions github-actions bot temporarily deployed to staging/pull/250/dartdoc November 30, 2021 13:06 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/250/dartdoc December 2, 2021 09:21 Inactive
Copy link
Contributor

@ikbalkaya ikbalkaya left a comment

Choose a reason for hiding this comment

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

Light review warning - I skimmed through changes as this is a significantly big PR. One think I realised was use of lots of statics. As this is a refactor work I assume there is a good reason for them, but I have to say that use of lots of statics makes me anxious.

@QuintinWillison QuintinWillison merged commit 4da3baa into main Dec 2, 2021
@QuintinWillison QuintinWillison deleted the enhancement/simplify-auth-callback-state branch December 2, 2021 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improved functionality.
Development

Successfully merging this pull request may close these issues.

4 participants