ably / ably-flutter

A wrapper around our Cocoa and Java client library SDKs, providing iOS and Android support for those using Flutter and Dart.
https://ably.com/download
Apache License 2.0
60 stars 16 forks source link

setOnBackgroundMessage() is now a Future<void> that will await for the function to complete. #498

Closed asoap closed 11 months ago

asoap commented 11 months ago

This is in regards to this issue here: https://github.com/ably/ably-flutter/issues/496

If a setOnBackgroundMessage() was fired and if it took to long to complete the isolate on Android would end before the code was completed. This was extra annoying if you were trying to debug a background messsage or if it used other packages that used async/await. Those items just wouldn't fire.

Now all code will complete before the isolate dismisses itself.

asoap commented 11 months ago

Do I need to go and fix these errors that have popped up? There is an AWS error for credentials, I'm not sure how I'm supposed to get past that for a github test.

ttypic commented 11 months ago

Do I need to go and fix these errors that have popped up? There is an AWS error for credentials, I'm not sure how I'm supposed to get past that for a github test.

No need, this is because actions run on forked repository and it doesn't have access to secrets

asoap commented 11 months ago

Thank you very much for updated version, looks good! But could you please run flutter format . there are some extra whitespaces and linter doesn't like it. Also I am not sure we need to do any changes in lib/src/platform/src/background_android_isolate_platform.dart

I added a comment above. I'm fairly sure that change is required. I can test without it if you like to confirm.

Also I'm a bit confused on how to run flutter format. Obviously the command line, but for these changes I made them locally in my installed packages for flutter, and I made a fork and changed them online. I guess I could pull my repo locally, run the command and then push it. But I'm not sure if that would work. Working on packages is all new to me.

ttypic commented 11 months ago

@asoap

I added a comment above. I'm fairly sure that change is required. I can test without it if you like to confirm.

It will be lovely if you test, but I am pretty sure that nothing will change if you rollback changes from lib/src/platform/src/background_android_isolate_platform.dart

I guess I could pull my repo locally, run the command and then push it

Yeah, this is exactly what I meant, it will work, just push changes in main branch on your fork.

Working on packages is all new to me.

Nice, my congratulations! :) If you have any questions we are happy to help

asoap commented 11 months ago

I pushed the changes to my repo. Also I was confused looking for the command "flutter format ." It turns out it was "dart format ."

I think everything should be good to go now.

asoap commented 11 months ago

Huzzah! Is this done now? Or do I need to do anything to resolve the conversation?

ttypic commented 11 months ago

@asoap Sorry, still some checks are not passing, so I can't merge it, could you add those changes:

lib/src/platform/src/push_notification_events_internal.dart

image

lib/src/platform/src/method_call_handler.dart

image
asoap commented 11 months ago

Can do. I'm going to be in the middle of some work, but I will try to address this today and test it out to make sure it works before submitting changes.

asoap commented 11 months ago

Unfortuantely work went long today, and I'll have to address this on Monday.

asoap commented 11 months ago

I tested these changes and everything is working as expected. I then pushed them to the pull request.

asoap commented 11 months ago

Hello @asoap I'm sorry but formatting checks are still failing and I can't merge. Do you mind if I close the PR and add required changes in new internal PR?

Yeah absolutely, do what you need to do.

ttypic commented 11 months ago

moved to https://github.com/ably/ably-flutter/pull/499