Migrating to null-safety #90
Conversation
There was a problem hiding this comment.
Thank you for the contribution. Please walk through the log event calls and make sure that we use required / "not nullable" where it is supposed to be. As opposed to adding support for non-nullable but making all parameters nullable, which effectively is the same as not adding the support.
| @@ -165,7 +164,7 @@ class FacebookAppEvents { | |||
| /// Parameter [registrationMethod] is used to specify the method the user has | |||
| /// used to register for the app, e.g. "Facebook", "email", "Google", etc. | |||
| /// See: https://developers.facebook.com/docs/reference/androidsdk/current/facebook/com/facebook/appevents/appeventsconstants.html/#eventnamecompletedregistration | |||
| Future<void> logCompletedRegistration({String registrationMethod}) { | |||
| Future<void> logCompletedRegistration({String? registrationMethod}) { | |||
There was a problem hiding this comment.
I think registrationMethod is required
/**
* This function assumes logger is an instance of AppEventsLogger and has been
* created using AppEventsLogger.newLogger() call.
*/
public void logCompleteRegistrationEvent (String registrationMethod) {
Bundle params = new Bundle();
params.putString(AppEventsConstants.EVENT_PARAM_REGISTRATION_METHOD, registrationMethod);
logger.logEvent(AppEventsConstants.EVENT_NAME_COMPLETED_REGISTRATION, params);
}There was a problem hiding this comment.
I just had a look at the source code of one of my app currently on the market, and I am not using the default parameter name for the signup method, which leads me to think that it is not really required. (I was using "method" instead of "fb_registration_method" and it worked for the past few months)
There was a problem hiding this comment.
Before null saftey it was a developer interpretation choice... but now with null safety it will be comedictated by the design of the plugin. So it would be good to find clear documentation from Facebook SDK definition.
There was a problem hiding this comment.
Hi @DennisAlund, I found out that an event's parameters are always optional! Of course, this is only true for the events being logged through the logEvent method. (https://developers.facebook.com/docs/app-events/overview)
So the registrationMethod parameter should not be required since that would mean adding a constraint that Facebook does not require.
There was a problem hiding this comment.
@GaspardMerten agree, here you can see the official documentation by Facebook in Swift
https://developers.facebook.com/docs/swift/reference/structs/appevent.html
public static func completedRegistration(registrationMethod:String?=nil, valueToSum:Double?=nil, extraParameters:ParametersDictionary=[:])| Method Name | Description |
|---|---|
| registrationMethod | Optional registration method used. |
| valueToSum | Optional value to sum. |
| extraParameters | Optional dictionary of extra parameters. |
|
@DennisAlund Would it be possible to merge this and create a new version? I would love to upgrade my project to flutter null-safety :) |
|
Any news on this? For now I'll just point my pub package to the changed branch. |
|
Hi, Thanks, |
|
@yanivshaked, I just did it :) Let's hope the issue was resolved on the upstream repo! |
|
Thanks for the super quick action! |
|
Issue seems to be solved 👍 👍 |
|
@DennisAlund Can we please get this merged in, if there is no issues? |
|
@DennisAlund This package is the only remaining blocker for my upgrade to flutter 2 with null safety. A review and merge would be greatly appreciated |
|
Sorry for delay. Let's try it out. |
|
Released in |
A PR to migrate this plugin to null safety.