Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[firebase_analytics] Allow setUserID input to be null #888

Merged
merged 1 commit into from
Nov 9, 2018

Conversation

rockwotj
Copy link
Contributor

@rockwotj rockwotj commented Nov 5, 2018

@rockwotj rockwotj changed the title Allow setUserID input to be null [firebase_analytics] Allow setUserID input to be null Nov 5, 2018
@kroikie kroikie self-assigned this Nov 8, 2018
@kroikie
Copy link
Contributor

kroikie commented Nov 9, 2018

@rockwotj Thanks for this PR and bringing the plugins closer to the native SDKs. However I think the handling of the user ID on the native side will need to be updated to handle the null case since currently it assumes that the ID coming over the channel is never null.

See the Android implementation.

  private void handleSetUserId(MethodCall call, Result result) {
    final String id = (String) call.arguments;
    firebaseAnalytics.setUserId(id);
    result.success(null);
  }

I think this would break if the user ID is null. Could you update the native handlers so they don't break when null is passed?

@rockwotj
Copy link
Contributor Author

rockwotj commented Nov 9, 2018

Hey @kroikie,

I don't think that actually breaks. It's totally valid to case null to String, see: https://repl.it/repls/ReadySleepyAttribute

Additionally the iOS implementation casts nil to NSString* which is also a valid cast.

And the flutter guide says they convert to those types.
https://flutter.io/docs/development/platform-integration/platform-channels

@kroikie
Copy link
Contributor

kroikie commented Nov 9, 2018

@rockwotj You are quite correct, thanks for clarifying that.

@kroikie kroikie merged commit 8388ee6 into flutter:master Nov 9, 2018
andreidiaconu pushed a commit to andreidiaconu/plugins that referenced this pull request Feb 17, 2019
andreidiaconu added a commit to andreidiaconu/plugins that referenced this pull request Feb 17, 2019
julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants