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

Use futures list internally to manage send events #184

Conversation

eliasyishak
Copy link
Contributor

@eliasyishak eliasyishak commented Oct 20, 2023

Related to issue:

This PR is adding an internal instance member _futures, which is a list of Future objects that are returned from the http client.

When the user of this package invokes analytics.send(...), they will no longer have to await the call which can delay them. However, at the end, the user of this package should call analytics.close() and await that call on exit to ensure events have been sent.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@@ -472,7 +476,10 @@ class AnalyticsImpl implements Analytics {
}

@override
void close() => _gaClient.close();
Future<void> close() async {
Copy link
Member

Choose a reason for hiding this comment

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

were we calling analytics.close() on the tool anyway?

I'm thinking (provided other clients aren't implementing this interface), an easier, non-breaking change to make would be to not change the signature of any existing methods, and just adding a new method, waitForLastPing({Duration? timeout}) that clients can choose to opt in to, if they want to ensure an event was sent, or ignore if they'd rather prioritize exiting as soon as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

were we calling analytics.close() on the tool anyway?

No, and we didn't really have to because we weren't sending events yet, but that is a good catch for the open PR for me to add in the close statement on exit.

non-breaking change to make would be to not change the signature of any existing methods

And i think this entire PR would be a breaking change regardless since the send methods return type has now changed

Copy link
Member

Choose a reason for hiding this comment

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

I'm saying if we DON'T change the return signature of any existing methods, it won't be a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see, you don't agree on having the send method return void makes sense now though? I understand it is a breaking change but I think this gets us more in line with the legacy analytics

Copy link
Member

Choose a reason for hiding this comment

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

I'm not saying it doesn't make sense, I'm just pointing out the minimal amount of engineering effort would likely be to avoid breaking other clients. If think it's reasonable to force existing clients to migrate, then I'm ok with a breaking change.

pkgs/unified_analytics/lib/src/analytics.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@andrewkolos andrewkolos left a comment

Choose a reason for hiding this comment

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

LGTM

@dart-lang dart-lang deleted a comment from github-actions bot Oct 24, 2023
@eliasyishak eliasyishak merged commit e3dd149 into dart-lang:main Oct 24, 2023
6 checks passed
@eliasyishak eliasyishak deleted the 183-implement-futures-list-internal-to-analytics-instance branch October 24, 2023 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants