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

Avoid void async ios #38

Closed
wants to merge 9 commits into from
Closed

Avoid void async ios #38

wants to merge 9 commits into from

Conversation

nkmrh
Copy link
Collaborator

@nkmrh nkmrh commented Feb 1, 2024

Summary

Implementation

  • iOS 対応分です

Test plan

Other Information

@nkmrh nkmrh requested a review from SojiroNishimura February 1, 2024 01:35
@nkmrh nkmrh self-assigned this Feb 1, 2024
@nkmrh nkmrh changed the base branch from master to avoid-void-async February 1, 2024 01:35
@@ -62,28 +62,31 @@ public class SwiftKarteCorePlugin: NSObject, FlutterPlugin {
switch methodName {
case "track":
if let name = arguments?["name"] as? String {
Tracker.track(name, values: values)
let task = Tracker.track(name, values: values)
task.completion = { _ in result(nil) }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

非同期を待てる api を利用するように変更しました

Copy link
Contributor

Choose a reason for hiding this comment

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

@nkmrh これって特に何もReturnしてないように見えるんですけど、呼び出し側をFutureにするだけじゃなくてこちらの対応も必要になる感じなんでしょうか?KarteAppのメソッドはFutureになってるだけだったのでこっちも同じじゃだめなのか気になりました

Copy link
Collaborator Author

@nkmrh nkmrh Feb 2, 2024

Choose a reason for hiding this comment

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

@SojiroNishimura ここは task の終了を待ってから result block を呼び bridging method の呼び出し完了を伝えています。この処理がない場合、flutter 側で await を記述しても、即座に bridging method の呼び出し完了となり、イベントの送信を待たない挙動となってしまうため、このようにしています。

Copy link
Collaborator

Choose a reason for hiding this comment

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

fluttter側のawaitにtimeoutって存在するんでしょうか?
ネットワーク環境やスレッドの状況によっては長時間帰ってこなかったり、
リクエスト中にプロセスがサスペンドした場合はどうなるか、気になりました。

調べればいい話をすみません。


あと、これはリクエストの成否を返さない変更ですよね?
TrackingTask自体は成否を返すので、合わせるなら成否を返すようにした方がいい気がしますが、その場合は互換性を破壊する、という判断でしょうか?
dartがライブラリをバイナリで参照していたら互換性なくなりそうですが、voidから別の型への変更なら互換性が保たれるのかな、と思ったりしていました。。
(もし違えばそもそもFutureで囲ってしまうことも互換性破壊になりますか?)

chatGPTに軽く質問した限りでは「voidは変数に入れられないので、自由度が上がるのみで互換性は壊さない」、というような回答を得ましたが、確証がありません。
dartの互換性保持がわかっておらず申し訳ありませんが、念のため詳しく調べていただけると助かります。

Copy link
Collaborator Author

@nkmrh nkmrh Feb 22, 2024

Choose a reason for hiding this comment

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

@wasnot こちら調査中ですが、一旦コメントさせていただきます 🙏

fluttter側のawaitにtimeoutって存在するんでしょうか?
ネットワーク環境やスレッドの状況によっては長時間帰ってこなかったり、
リクエスト中にプロセスがサスペンドした場合はどうなるか、気になりました。

timeout 指定は .timeout() できるようです。

Future<String> myFuture = Future.delayed(Duration(seconds: 5), () {
  // 何か時間がかかる処理
  return '結果';
});

try {
  // Futureが完了するのを最大3秒間待機
  String result = await myFuture.timeout(Duration(seconds: 3));
  print(result); // タイムアウトしなければ結果が出力される
} catch (e) {
  // タイムアウトまたは他のエラーが発生した場合の処理
  print('エラーまたはタイムアウト: $e');
}

あと、これはリクエストの成否を返さない変更ですよね?

はい。

TrackingTask自体は成否を返すので、合わせるなら成否を返すようにした方がいい気がしますが、その場合は互換性を破壊する、という判断でしょうか?

そのように考えていました。このように completion ブロックの中で result に nil を渡して呼ばないと dart 上では await を使って結果を待っているように見えても、実際の挙動はプラグインの実装上、ネイティブコードの実行結果を待っていないのでこのように、待つ形にしました。

(もし違えばそもそもFutureで囲ってしまうことも互換性破壊になりますか?)

一般的にはメソッドシグネチャが変わる変更は互換性破壊になるかと思います。

dartの互換性保持がわかっておらず申し訳ありませんが、念のため詳しく調べていただけると助かります。

メソッドの使用方法と期待される挙動が変わるため破壊的変更になるかと思います。

  • voidを返すメソッドは、通常、副作用のみを期待される
  • これが別の型を返すようになると、そのメソッドの使い方や目的が根本的に変わる

メソッドシグネチャが変わるとコンパイル結果も変わる可能性があるため互換性破壊になると言えそうです。

varで受け取っても同じ。dynamicで受け取ればnullとして扱うことはできるので、このパターンが問題になるかも。。

こちら試してみたところ、以下のエラーが発生しました。

void hoge() {}
dynamic v = hoge();
Screenshot 2024-02-22 at 9 10 23

一旦、実装・挙動は変えずに、void -> Future<Void> の変更だけ適用して、メジャーバージョンを上げておく、というのが安牌でしょうか。

@@ -24,7 +24,7 @@ const WrapperChannel _channel = const WrapperChannel('karte_notification');

/// リモート通知メッセージのパースおよびメッセージ中に含まれるディープリンクのハンドリングを行うためのクラスです。
class Notification {
static void registerFCMToken(String fcmToken) async {
static Future<void> registerFCMToken(String fcmToken) async {
Copy link
Collaborator Author

@nkmrh nkmrh Feb 1, 2024

Choose a reason for hiding this comment

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

@SojiroNishimura karte_core 同様に非同期対応した方が良いでしょうか?もしそうであれば、NativeSDK の方も対応します。

  • karte_notification(NativeSDK に Task を返すように修正が必要です)
  • karte_variables(NativeSDK に Task を返すように修正が必要です)

Copy link
Collaborator Author

@nkmrh nkmrh Feb 2, 2024

Choose a reason for hiding this comment

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

trackOpen, trackClick についてはネイティブ実装が、配列を送信できる api となっているため、単純に task を返す修正だけでは難しそうでした。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

registerFCMToken は具体的には下記の変更を行います。

commit 56df6f88c73889e6da787c3092a14b2d934df12b
Author: nkmrh <[email protected]>
Date:   Sat Feb 3 07:04:12 2024 +0900

    Add completion argument

diff --git a/KarteRemoteNotification/FCMTokenRegistrar.swift b/KarteRemoteNotification/FCMTokenRegistrar.swift
index 5250f14..2698eda 100644
--- a/KarteRemoteNotification/FCMTokenRegistrar.swift
+++ b/KarteRemoteNotification/FCMTokenRegistrar.swift
@@ -36,20 +36,25 @@ internal class FCMTokenRegistrar {
     }
 
     func registerFCMToken() {
-        registerFCMToken(notificationSettingsProvider.fcmToken)
+        registerFCMToken(notificationSettingsProvider.fcmToken, completion: nil)
     }
 
-    func registerFCMToken(_ token: String?) {
+    func registerFCMToken(_ token: String?, completion: TrackCompletion?) {
         guard let token = token else {
+            completion?(false)
             return
         }
 
         notificationSettingsProvider.checkAvailability { [weak self] subscribe in
             guard let self = self, self.isChanged(token: token, subscribe: subscribe) else {
+                completion?(false)
                 return
             }
 
-            Tracker.track(event: Event(.pluginNativeAppIdentify(subscribe: subscribe, fcmToken: token)))
+            let task = Tracker.track(event: Event(.pluginNativeAppIdentify(subscribe: subscribe, fcmToken: token)))
+            if let completion {
+                task.completion = completion
+            }
 
             self.subscribe = subscribe
             self.token = token
diff --git a/KarteRemoteNotification/KRTApp+RemoteNotification.m b/KarteRemoteNotification/KRTApp+RemoteNotification.m
index 414c287..9c1ef3a 100644
--- a/KarteRemoteNotification/KRTApp+RemoteNotification.m
+++ b/KarteRemoteNotification/KRTApp+RemoteNotification.m
@@ -24,8 +24,8 @@
 
 @implementation KRTApp (RemoteNotification)
 
-+ (void)registerFCMToken:(NSString *)fcmToken {
-    [ObjcCompatibleScopeForNotification registerFCMToken:fcmToken];
++ (void)registerFCMToken:(NSString *)fcmToken completion:(void (^ _Nullable)(BOOL))completion {
+    [ObjcCompatibleScopeForNotification registerFCMToken:fcmToken completion:completion];
 }
 
 @end
diff --git a/KarteRemoteNotification/KarteApp+RemoteNotification.swift b/KarteRemoteNotification/KarteApp+RemoteNotification.swift
index 97e827a..2e3a997 100644
--- a/KarteRemoteNotification/KarteApp+RemoteNotification.swift
+++ b/KarteRemoteNotification/KarteApp+RemoteNotification.swift
@@ -23,7 +23,7 @@ public extension KarteApp {
     /// なお初期化が行われていない状態で呼び出した場合は登録処理は行われません。
     ///
     /// - Parameter fcmToken: FCMトークン
-    class func registerFCMToken(_ fcmToken: String?) {
-        ObjcCompatibleScopeForNotification.registerFCMToken(fcmToken)
+    class func registerFCMToken(_ fcmToken: String?, completion: TrackCompletion?) {
+        ObjcCompatibleScopeForNotification.registerFCMToken(fcmToken, completion: completion)
     }
 }
diff --git a/KarteRemoteNotification/ObjcCompatibleScope.swift b/KarteRemoteNotification/ObjcCompatibleScope.swift
index b0f1d50..439e776 100644
--- a/KarteRemoteNotification/ObjcCompatibleScope.swift
+++ b/KarteRemoteNotification/ObjcCompatibleScope.swift
@@ -21,8 +21,8 @@ import Foundation
 /// **SDK内部で利用するクラスであり、通常のSDK利用でこちらのクラスを利用することはありません。**
 public class ObjcCompatibleScopeForNotification: NSObject {
     @objc
-    public static func registerFCMToken(_ fcmToken: String?) {
-        FCMTokenRegistrar.shared.registerFCMToken(fcmToken)
+    public static func registerFCMToken(_ fcmToken: String?, completion: TrackCompletion?) {
+        FCMTokenRegistrar.shared.registerFCMToken(fcmToken, completion: completion)
     }
 
     @objc

(native 実装にタスク完了 completion block を追加)

case "registerFCMToken":
                if let token = (call.arguments as? [String:Any?])?["fcmToken"] as? String {
                    KarteApp.registerFCMToken(token) { _ in
                        result(nil)
                    }
                }

(plugin native 実装側で block を利用)

Copy link
Collaborator

@wasnot wasnot Feb 13, 2024

Choose a reason for hiding this comment

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

Futureで囲うなら対応した方が良さそうですが、Native側でも現状非対応であるならば、今は無理にしなくても良いように思います。
また、互換性観点からも、一度Futureに変更した後にFutureへ互換性を保持したまま変更できないのであれば、無理に今変更しない方が良いと思います。

Native側の変更に関しては、変更案ではregister済みの場合にfalseを返していますが、現状それが正しいのか判断がつきません。
(register済みはregister成功ではないのか?)
なので、もし変更する場合は、その仕様を他社SDKや一般的なライブラリを参照して妥当性を確認した上で決めたいです。

sdks:
dart: ">=3.1.0-185.0.dev <4.0.0"
dart: ">=3.0.0-0 <4.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

dartのバージョンが低くて他のパッケージがバージョンダウンされてそうなのでdartのバージョン上げて試してもらえないでしょうか。3.2にしてもらって問題なければ3.2で、だめそうなら3.1系の最新でお願いします
https://dart.dev/guides/language/evolution#dart-31

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

77c4cf8で対応し 3.2 に変更しました

@SojiroNishimura
Copy link
Contributor

@wasnot こちら一通り見てみたんですが念のためチェックお願いします 🙏

Copy link
Collaborator

@wasnot wasnot left a comment

Choose a reason for hiding this comment

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

@nkmrh 対応ありがとうございます!

コメントしましたが、単なる変更だけでなく互換性の観点からの動作や仕様の確認を念入りにお願いしたいです。
こちらの不勉強のため、お手数おかけしていますが、よろしくお願いします。

@@ -62,28 +62,31 @@ public class SwiftKarteCorePlugin: NSObject, FlutterPlugin {
switch methodName {
case "track":
if let name = arguments?["name"] as? String {
Tracker.track(name, values: values)
let task = Tracker.track(name, values: values)
task.completion = { _ in result(nil) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

fluttter側のawaitにtimeoutって存在するんでしょうか?
ネットワーク環境やスレッドの状況によっては長時間帰ってこなかったり、
リクエスト中にプロセスがサスペンドした場合はどうなるか、気になりました。

調べればいい話をすみません。


あと、これはリクエストの成否を返さない変更ですよね?
TrackingTask自体は成否を返すので、合わせるなら成否を返すようにした方がいい気がしますが、その場合は互換性を破壊する、という判断でしょうか?
dartがライブラリをバイナリで参照していたら互換性なくなりそうですが、voidから別の型への変更なら互換性が保たれるのかな、と思ったりしていました。。
(もし違えばそもそもFutureで囲ってしまうことも互換性破壊になりますか?)

chatGPTに軽く質問した限りでは「voidは変数に入れられないので、自由度が上がるのみで互換性は壊さない」、というような回答を得ましたが、確証がありません。
dartの互換性保持がわかっておらず申し訳ありませんが、念のため詳しく調べていただけると助かります。

@@ -24,7 +24,7 @@ const WrapperChannel _channel = const WrapperChannel('karte_notification');

/// リモート通知メッセージのパースおよびメッセージ中に含まれるディープリンクのハンドリングを行うためのクラスです。
class Notification {
static void registerFCMToken(String fcmToken) async {
static Future<void> registerFCMToken(String fcmToken) async {
Copy link
Collaborator

@wasnot wasnot Feb 13, 2024

Choose a reason for hiding this comment

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

Futureで囲うなら対応した方が良さそうですが、Native側でも現状非対応であるならば、今は無理にしなくても良いように思います。
また、互換性観点からも、一度Futureに変更した後にFutureへ互換性を保持したまま変更できないのであれば、無理に今変更しない方が良いと思います。

Native側の変更に関しては、変更案ではregister済みの場合にfalseを返していますが、現状それが正しいのか判断がつきません。
(register済みはregister成功ではないのか?)
なので、もし変更する場合は、その仕様を他社SDKや一般的なライブラリを参照して妥当性を確認した上で決めたいです。

@nkmrh nkmrh closed this Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants