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

Propose for 0.4.1 improvements #13

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

GabrielRozendo
Copy link
Contributor

@GabrielRozendo GabrielRozendo commented Sep 3, 2024

Quoting changelog.md

0.4.1

  • Handle the lifecycle using AppLifecycleListener instead SystemChannels.lifecycle.setMessageHandler
  • Changed trackEvent to not future
    Avoiding calling unawaited every time you want to track a new event.
    The lint rule: https://dart.dev/tools/linter-rules/unawaited_futures
  • Fix the error message when appKey is invalid
  • Update example with latest flutter version and an "app opened" event
  • Fix web info

1. Lifecycle issue

The PR #8 is working as should

but now I need to listen the flutter's lifecycle in my app (not in aptabase's package) and since the package already is doing that and as documented on SystemChannels.lifecycle:

/// The given callback will replace the currently registered callback for this
/// channel, if any. To remove the handler, pass null as the handler
/// argument.

we would lose the feature "send events when app goes to background".
And I've found out the SystemChannels.lifecycle has other unwanted side effects in general.

2. Changed trackEvent to not future

I use the unawaited_futures lint rule so I should add the unawaited every time I want to track and already create a caller just to wrap it. Thinking in other users and in this lint, changed it to not future anymore.

The problem here is if the dev already implemented either await or unawaited when calling the trackEvents, it'd be an error (as it is not future anymore) and will break (so shouldn't be only minor bump 0.4.1). I prefer to avoid that.

One solution would be implement another caller as not future and (maybe) add a deprecated in future caller.
I really want to hear you about that before.

3. Fix web info

In web is flagging OS as unknown.
In package DeviceInfoPlugin has the implementation for web platform, check out in pub.dev example

Already change to use the webBrowserInfo

CleanShot 2024-09-03 at 13 09 36

The issue here is the version part as the web provides not a clear single option to use.

In documentation has these info:

 Map<String, dynamic> _readWebBrowserInfo(WebBrowserInfo data) {
    return <String, dynamic>{
      'browserName': data.browserName.name,
      'appCodeName': data.appCodeName,
      'appName': data.appName,
      'appVersion': data.appVersion,
      'deviceMemory': data.deviceMemory,
      'language': data.language,
      'languages': data.languages,
      'platform': data.platform,
      'product': data.product,
      'productSub': data.productSub,
      'userAgent': data.userAgent,
      'vendor': data.vendor,
      'vendorSub': data.vendorSub,
      'hardwareConcurrency': data.hardwareConcurrency,
      'maxTouchPoints': data.maxTouchPoints,
    };
  }

Here is my debug value:
CleanShot 2024-09-03 at 12 55 04@2x

We could use the appVersion but still not so clear and simple.
And yet we should limit it to 100 max chars as server defined.
39: osInfo.version.substring(0, 100),

Again I want to know what your opinion before move on.


Sorry for long message but want to discuss before open the PR itself and also if you want to, I can break the PRs in 1,2,3 parts if you'd prefer.

Avoiding calling `unawaited` every time you want to track a new event.
The lint rule: https://dart.dev/tools/linter-rules/unawaited_futures
… latest flutter version

Warning: In index.html:37: Local variable for "serviceWorkerVersion" is deprecated. Use "{{flutter_service_worker_version}}" template token instead. See https://docs.flutter.dev/platform-integration/web/initialization for more details.
Warning: In index.html:46: "FlutterLoader.loadEntrypoint" is deprecated. Use "FlutterLoader.load" instead. See https://docs.flutter.dev/platform-integration/web/initialization for more details.
@GabrielRozendo
Copy link
Contributor Author

Already tested on iOS, Mac, Web and Android.
CleanShot 2024-09-03 at 13 19 35

@GabrielRozendo GabrielRozendo marked this pull request as ready for review October 29, 2024 19:19
@GabrielRozendo GabrielRozendo changed the title [WIP] Propose for 0.4.1 improvements Propose for 0.4.1 improvements Oct 29, 2024
@GabrielRozendo GabrielRozendo mentioned this pull request Oct 29, 2024
@manuthebyte
Copy link

Handle the lifecycle using AppLifecycleListener instead SystemChannels.lifecycle.setMessageHandler

Great and veeerrrryyy important change. The big issue about the approach before is that you set the message handler to your method for the whole app. The rest of the app will not be able to receive app lifecycle events.

@GabrielRozendo
Copy link
Contributor Author

The big issue about the approach before is that you set the message handler to your method for the whole app. The rest of the app will not be able to receive app lifecycle events.

Indeed. I realized one day after merged 😔
You could said it before 😅🥲

What else can you comment?

@manuthebyte
Copy link

manuthebyte commented Nov 5, 2024

Indeed. I realized one day after merged 😔

Thats unfortunate 😞

You could said it before 😅🥲

I just realized it when we had this bug in one of our apps. I was thinking "Why can't I receive lifecycle events" until we bruteforce-removed every plugin until we knew it was aptabase 😆
Just saw it in the code.

What else can you comment?

What do you mean? Feedback for the rest of the code? 😄

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.

2 participants