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
feat(firebase_analytics): allow custom parameters (strings and numbers) for events and event items #11030
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Can you also add an optional items array parameter to the logEvent method? Adding the parameters map to the existing functions is great but does not future proof this change. Having maximum flexibility on logEvent allows users to keep up with the ever changing GA4 recommendations. |
I would avoid it because arrays are explicitly forbidden in GA4 (I don't find the piece of documentation right now, sorry). |
@@ -6,8 +6,7 @@ part of firebase_analytics; | |||
|
|||
/// Firebase Analytics API. | |||
class FirebaseAnalytics extends FirebasePluginPlatform { | |||
FirebaseAnalytics._({required this.app}) | |||
: super(app.name, 'plugins.flutter.io/firebase_analytics'); | |||
FirebaseAnalytics._({required this.app}) : super(app.name, 'plugins.flutter.io/firebase_analytics'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this (and other similar changes) depends on specific IDE settings and could be reversed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
packages/firebase_analytics/firebase_analytics/lib/src/firebase_analytics.dart
Outdated
Show resolved
Hide resolved
I've just done what you requested. Let me know if you have further suggestions in order to merge. Thanks |
@muccy - do you mind merging with master branch, please? This PR has the green light, I'm hoping that merging with master will also trigger the e2e tests which haven't appeared to run and I'm not quite sure why that is. |
Merged upstream/master. Tell me if I can provide further help |
@muccy - do you mind running
A couple of files in your PR are not formatted correctly: https://github.com/firebase/flutterfire/actions/runs/6799184595/job/18486372026?pr=11030#step:8:26 |
Sure: command ran and pushed to master |
@muccy - could you remove any formatting changes to files unrelated to this PR? Formatting has changed to firestore files which is probably because you aren't using latest clang-format and swiftformat tools. Either way, you can simply revert changes directly in the Github "files changed" tab and clicking on the 3 dots button in the top right hand corner of the file you wish to revert and click "revert changes". This will directly commit to remote, you will need to pull to local to keep in sync with remote 👍 |
Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @muccy. Very much appreciated 🙏
Description
The version 10.1 of
firebase_analytics
plugin introduced an assertion to filter out lists from custom parameters (see: #9520). This change forces us to use stock methods (e.g.:logPurchase
) in order to track e-commerce events but we are not allowed to pass extra (legal) parameters to GA4.This PR adds
parameters
field to pass extra data along stock GA4 events.Related Issues
Closes #10629
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
).This will ensure a smooth and quick review process. Updating the
pubspec.yaml
and changelogs is not required.///
).melos run analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?