flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
164.72k stars 27.15k forks source link

Should we be using GGLContext (iOS) in plugins to configure Google services? #10819

Closed mehmetf closed 7 years ago

mehmetf commented 7 years ago

GGLContext is provided as a utility that configures Google services (signin, analytics, gcm etc). However, do we really need it?

I propose that we get rid of this dependency and entirely rely on clientId being passed in from Dart side for the plugins that need it.

mehmetf commented 7 years ago

@tvolkert @goderbauer for comments.

mehmetf commented 7 years ago

I tracked down a bit more interesting history for this:

tvolkert commented 7 years ago

I think @collinjackson knows the history here as to why we read the clientId from GoogleServices-Info.plist vs having the developer specify it in their Dart code.

tvolkert commented 7 years ago

Also /cc @mit-mit

mit-mit commented 7 years ago

I think we rely on GoogleService-Info.plist as that is the workflow that the various Google consoles for registering your app provides, e.g. https://firebase.google.com/docs/ios/setup

mehmetf commented 7 years ago

Thanks Michael. The link you shared only applies to Firebase family of plugins. Google Sign In is a bit of an oddball. It pretends to be in that family but it really isn't. It does not get configured by [FIRApp configure]; command which is typically what you would do to configure Firebase components. It also requires a lot of additional parameters to properly initialize (such as scopes). Compare that to Firebase Auth plugin which is properly integrated to Firebase workflow.

I think we should not mix these two worlds. Yes, it is tempting to provide user the smoothest experience possible but in this particular scenario, I don't think it is making much of a difference due to additional parameters required. Thus, it is making our own lives harder for not much gain.

If a google (or external) app is using google_signin, there's non-negligible chance they are not using Firebase at all and thus won't have a GoogleService-Info.plist. Then, it actually makes users lives harder because it is not immediately obvious why the plugin does not work.

mit-mit commented 7 years ago

Doesn't the Google sign-in developer workflow rely on that same file? https://developers.google.com/identity/sign-in/ios/start

collinjackson commented 7 years ago

If you're using the Firebase plugin, the way to configure it is to provide a GoogleService-Info.plist, so it would be an extra step to make developers copy information out of that file and hard code it into their Dart code for Google Sign In. If they have different configuration files for different platforms (iOS/Android) and different types of builds (production/enterprise/dev), it seems like some devs might prefer to have their Dart code be oblivious to the flavor of build and have configuration occur in native-land.

I agree that if you're not using that plist file, it would be nice to not have to depend on GGLContext, although I suspect that most external devs who just want to use the plugin and have it work don't care either way.

Some options:

  1. Split off the code that calls GGLContext configureWithError: into a separate category or delegate class that is stubbed out or not implemented in the target for internal builds. This would enable us to keep the internal and external versions "in sync" while one depends on GGLContext and the other doesn't.
  2. Have a separate "gglcontext" plugin that parses the GoogleService-Info.plist for you if you add it to pubspec.yaml. This would only be necessary on iOS and do nothing on Android.
  3. We could look for GoogleService-Info.plist file ourselves and parse the clientID out of it without using GGLContext.
mehmetf commented 7 years ago

If Michael is correct and GoogleService-Info.plist is the way to go to configure any Google service (Firebase or not), then what makes sense to me the most is to provide this as the only option to get client id.

Let's decide based on the internal discussion on the availability of GGLContext.

I will follow up on this with @tvolkert internally.

mehmetf commented 7 years ago

Additional details came into light (specifically Google cocoapod deprecation and the subsequent internal library deprecation) that made us reconsider this. Todd is leaning towards what Collin's (3) proposal especially if it is cheap to implement. We can declare GoogleService the canonical way of defining client-id for GoogleSignIn and use our own code to configure the library. Assigning to Todd since this blocks the internal push of GoogleSignIn.

github-actions[bot] commented 3 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.