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

Synchronous API for creating instances and Hot-reload fix #18

Merged
merged 15 commits into from
Jul 21, 2020

Conversation

tiholic
Copy link
Contributor

@tiholic tiholic commented Jun 27, 2020

This PR mainly focuses on creating Ably instances in a synchronous manner.
The following changes were made possible as all other operations on each ably instances is an asynchronous operation.

If not async? then, Sync + Lazy = Slazy!!! 😉

  1. All platform calls from dart side are made lazy
  2. Singleton Ably library instances on Android side (iOS will be caught up)

A peek on new APIs

import 'package:ably_flutter_plugin/ably.dart' as ably;

final clientOptions = ably.ClientOptions.fromKey("<YOUR APP KEY>");
clientOptions.logLevel = ably.LogLevel.verbose; //optional
ably.Rest rest = ably.Ably.Rest(options: clientOptions);

function testRest(){
  ably.RestChannel channel = rest.channels.get('test');
  await channel.publish(name: "Hello", data: "Ably");
}

function testRealtime(){
  ably.Realtime realtime = ably.Ably.Realtime(options: clientOptions);
    realtime.connection.on().listen((ably.ConnectionStateChange stateChange) async {
    print('Realtime connection state changed: ${stateChange.event}');
  });
}

Hot-reload issue fix 🌶️ ♻️ 🎉 🎉

  1. registerAbly is now used to dispose off any registered platform instances
  2. Android StreamsChannel source is replicated and necessary changes are made to dispose all listeners on hot-reload (Will be removing the streams_channel plugin dependency eventually)

NOTE: Draft PR supporting Android only.

@QuintinWillison @mattheworiordan @paddybyers Please share your feedback on the API. Will make necessary changes and proceed with implementing iOS platform related changes.

Copy link
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

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

Hi @tiholic ... thanks for your work on this.

Looking pretty good but there are a few things:

  1. I see that pubspec.lock has changed but there's no corresponding change to pubspec.yaml... I'm presuming that's because checking in this file was overlooked from a previous PR? Can you keep an eye on this if that is the case as it makes it impossible to look at this PR atomically in respect of the impact it has on the codebase as a whole.
  2. Given the nature of plugin development, as I think I may have mentioned a while back, we need to see changes to Java, iOS and the Dart code at the same time - making the PR deliver a feature completely across all runtimes. I'm worried about approving this PR when we have no idea whether this is replicable in Cocoa.
  3. In StreamsChannel.java (acknowledging it is somebody else's code) there is inconsistency around the use of final on args to methods. I also had another concern which I commented on directly in that file. This makes me worry a little about the quality of that code. Was it unit tested, for example, where it came from?

I'm going to continue noodling around locally, but I am yet to work out how to actually build the plugin from the command line (i.e. not just run it within the example app). What am I missing? Could we have some instructions somewhere for this, perhaps in the developer notes for now?

Also, one of the things I put in place early on was pedantic. Some of the minor spacing issues and stuff like that I can see in this PR should have been picked up by that. Can I run that from the command line also? It should be running, of course, on every build.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
test/ably_flutter_plugin_test.dart Outdated Show resolved Hide resolved
lib/src/impl/platform_object.dart Outdated Show resolved Hide resolved
lib/src/ably_implementation.dart Outdated Show resolved Hide resolved
lib/src/ably_implementation.dart Outdated Show resolved Hide resolved
Base automatically changed from feature/codec-upgrade to master July 6, 2020 18:59
tiholic added 4 commits July 7, 2020 01:30
## Sync + Lazy = Slazy!!! 😉
1. All platform calls from dart side are made lazy
2. Singleton Ably library instances on Android side (iOS will be caught up)

## Hot-reload issue
3. register ably is now used to dispose off any registered platform instances
4. StreamsChannel source is duplicated on android side and necessary changes are made to make hot-reload work well
@tiholic tiholic force-pushed the feature/synchronous-specification branch from da5de0f to 6582afe Compare July 6, 2020 20:01
@QuintinWillison
Copy link
Contributor

Thanks for the responses to my review comments, @tiholic.

I think there are still a few improvements before we can merge this. Thanks.

and a timeout to rule it out in case we never hear back from platform
 side
@tiholic
Copy link
Contributor Author

tiholic commented Jul 7, 2020

@QuintinWillison Yeah, this PR is still in draft state. I just getting some inputs on how the final API should look like.
Now that we have enough information to proceed further, I will finish iOS implementation and try to address all the unresolved comments from above.

tiholic added 6 commits July 11, 2020 23:06
import 'package:ably_flutter_plugin/ably.dart' as ably;

ably.Rest and ably.Realtime are now available directly to interact with
instead of a scoping inside a singleton class
## Sync + Lazy = Slazy!!!
1. Singleton AblyFlutter instance on iOS side

## Hot-reload issue
2. register ably is now used to dispose off any registered platform instances
3. StreamsChannel source is duplicated on iOS side and necessary changes are made to make hot-reload work well
1. Update example code to suit well with new API
2. respective lock files updated after removing streams_channel
@tiholic
Copy link
Contributor Author

tiholic commented Jul 12, 2020

@QuintinWillison iOS side caught up, if the runway is clear for landing, we can merge this.

Regarding the pubspec.lock changes, that was probably due to automatic minor version upgrade behavior.
^A.B.C will upgrade packages if minor version releases are available. If we need strict version for dependencies, we can go with package: 'A.B.C' instead of package: '^A.B.C' (ref: https://dart.dev/tools/pub/dependencies)

Updated DeveloperNotes with instructions on static code analysis (includes all build time checks)

@tiholic tiholic marked this pull request as ready for review July 12, 2020 18:14
@tiholic tiholic requested a review from QuintinWillison July 12, 2020 18:14
@QuintinWillison
Copy link
Contributor

Regarding pubspec.lock changes, I'm happy to leave things as they are for now and thank you for explaining. 😁

There is perhaps a wider discussion to be had here at Ably, across all client libraries, around dependency version management / auto management.

ios/Classes/AblyFlutter.h Outdated Show resolved Hide resolved
ios/Classes/AblyFlutter.h Outdated Show resolved Hide resolved
ios/Classes/AblyFlutter.m Show resolved Hide resolved
ios/Classes/AblyFlutterPlugin.m Outdated Show resolved Hide resolved
ios/Classes/AblyStreamsChannel.h Outdated Show resolved Hide resolved
ios/Classes/AblyStreamsChannel.m Outdated Show resolved Hide resolved
ios/Classes/AblyStreamsChannel.m Outdated Show resolved Hide resolved
ios/Classes/AblyStreamsChannel.m Show resolved Hide resolved
ios/Classes/AblyFlutterMessage.h Outdated Show resolved Hide resolved
DeveloperNotes.md Outdated Show resolved Hide resolved
tiholic and others added 3 commits July 20, 2020 20:06
#18 (review)

1. rename singleton returning static method to `sharedInstance`
2. remove `dispose` from header to make it private
3. code indents/formatting
4. remove `self->` from `AblyStreamsChannel` wherever not required
5. improve DeveloperNotes.md
Copy link
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

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

Happy for you to land this when you are ready, @tiholic. Thanks for all the work you've put in to resolve comments.

@tiholic tiholic merged commit 0c2f707 into master Jul 21, 2020
@tiholic tiholic deleted the feature/synchronous-specification branch July 21, 2020 11:56
tiholic added a commit that referenced this pull request Jul 21, 2020
#18 (review)

1. rename singleton returning static method to `sharedInstance`
2. remove `dispose` from header to make it private
3. code indents/formatting
4. remove `self->` from `AblyStreamsChannel` wherever not required
5. improve DeveloperNotes.md
@tiholic tiholic linked an issue Aug 10, 2020 that may be closed by this pull request
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.

Deviation from Ably SDK APIs
5 participants