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. |
muccy
changed the title
|
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.
I guess this (and other similar changes) depends on specific IDE settings and could be reversed.
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! |
russellwheatley
changed the title
Description
The version 10.1 of
firebase_analyticsplugin 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
parametersfield 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.yamland 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?