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

[google_sign_in] Support Dart-based configuration and serverClientId #5250

Merged
auto-submit merged 1 commit into from
Jul 7, 2022
Merged

[google_sign_in] Support Dart-based configuration and serverClientId #5250

auto-submit merged 1 commit into from
Jul 7, 2022

Conversation

@blaugold
Copy link
Contributor

@blaugold blaugold commented Apr 13, 2022 •
edited
Loading

This PR removes the requirement to add a GoogleService-Info.plist file for iOS apps and instead allows for configuration though Dart.

To this end, a new parameter (serverClientId) is added to the GoogleSignIn constructor. This necessitates a breaking change in google_sign_in_platform_interface.

While working on this PR, I noticed that on Android clientId was interpreted as a server client ID as opposed to an app client ID, like on iOS and Web. On Android, app client IDs are not required/supported. To provide an API that is consistent across platforms, passing a non-null value for clientId now throws logs a warning on Android and serverClientId is used as clientId was previously.

Closes flutter/flutter#96391
Fixes flutter/flutter#57945

Todo

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@blaugold blaugold requested a review from ditman as a code owner April 13, 2022 11:59
@radiKal07
Copy link

radiKal07 commented Apr 13, 2022

Wow, thanks a lot for this PR, I really need this in my app. Hopefully it will be merged soon.

@ditman
Copy link
Member

ditman commented Apr 13, 2022

This'd also fix flutter/flutter#57945 (added to the description)

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Apr 13, 2022

This necessitates a breaking change in google_sign_in_platform_interface.

Why does this require a breaking change, rather than following our usual documented process of adding a new method with the new parameter?

@blaugold
Copy link
Contributor Author

blaugold commented Apr 13, 2022

Why does this require a breaking change, rather than following our usual documented process of adding a new method with the new parameter?

You're right, this does not require a breaking change. What I meant is that this requires changes in the platform interface and I decided to make it a breaking change since all platform implementations are updated together with it. But I'm happy to refactor it to a non-breaking change.

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Apr 13, 2022

What I meant is that this requires changes in the platform interface and I decided to make it a breaking change since all platform implementations are updated together with it.

See https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#breaking-changes-to-plugin-platform-interfaces and https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-platform-interface-method-parameters

@blaugold blaugold mentioned this pull request Apr 13, 2022
Merged
11 tasks
@blaugold
Copy link
Contributor Author

blaugold commented Apr 14, 2022

I have added a list of todo items for implementing this to the description.

@blaugold blaugold force-pushed the blaugold/google_sign_in/dart-config branch from 655f993 to b9134b1 Compare April 14, 2022 15:00
@ditman ditman requested a review from cyanglaz April 14, 2022 17:59
@ditman
Copy link
Member

ditman commented Apr 14, 2022

/cc'ing @cyanglaz here too, he was interested in getting this feature implemented not too long ago!

cyanglaz
cyanglaz suggested changes Apr 14, 2022
View reviewed changes
@cyanglaz cyanglaz mentioned this pull request Apr 14, 2022
Merged
11 tasks
@blaugold blaugold force-pushed the blaugold/google_sign_in/dart-config branch from ffd22e8 to 178cfbb Compare April 15, 2022 16:25
@blaugold blaugold changed the title [google_sign_in] Support Dart-only configuration [google_sign_in] Support Dart-based configuration and serverClientId Apr 15, 2022
cyanglaz
cyanglaz suggested changes Apr 15, 2022
View reviewed changes
@jmagman
Copy link
Member

jmagman commented Apr 25, 2022

@cyanglaz looks like the changes were made, friendly ping

cyanglaz
cyanglaz suggested changes Apr 25, 2022
View reviewed changes
cyanglaz
cyanglaz reviewed Apr 26, 2022
View reviewed changes
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on iOS. Still need to remove the path dependency before merging.

@blaugold blaugold mentioned this pull request Apr 28, 2022
Closed
@blaugold
Copy link
Contributor Author

blaugold commented Apr 28, 2022

@stuartmorgan Do I need someone else to review and approve this PR before landing the platform interface PR?

stuartmorgan-g
stuartmorgan-g suggested changes May 2, 2022
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments below; mostly this looks good, but the Java test coverage issue needs to be resolved before we can start landing parts of the PR.

@flutter-dashboard
Copy link

flutter-dashboard bot commented May 3, 2022

This pull request is not mergeable in its current state, likely because of a merge conflict. Pre-submit CI jobs were not triggered. Pushing a new commit to this branch that resolves the issue will result in pre-submit jobs being scheduled.

@DominikKunadt
Copy link

DominikKunadt commented May 9, 2022

We are waiting on this to get merged is there any blocker left?

@jmagman
Copy link
Member

jmagman commented Jun 15, 2022

@blaugold this looks ready to merge once the package is no longer referenced locally and the publishable/federated_safety tests pass.

@blaugold blaugold force-pushed the blaugold/google_sign_in/dart-config branch from bddf436 to 5166457 Compare June 23, 2022 14:58
@blaugold blaugold mentioned this pull request Jun 23, 2022
Merged
11 tasks
@cedvdb cedvdb mentioned this pull request Jun 24, 2022
Closed
@cyanglaz
Copy link
Contributor

cyanglaz commented Jun 29, 2022

Waiting for #6034 to land

@loolooii loolooii mentioned this pull request Jul 6, 2022
Closed
@blaugold blaugold force-pushed the blaugold/google_sign_in/dart-config branch from 5166457 to bf421e1 Compare July 7, 2022 16:40
@blaugold
Copy link
Contributor Author

blaugold commented Jul 7, 2022

Platform implementations have landed and have been removed from this PR, so it should be good to go.

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Jul 7, 2022

Re-running tests now; it looks like the PR update was just posted before the previous PRs plugins had actually been published.

@stuartmorgan-g stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 7, 2022
@auto-submit auto-submit bot merged commit d07de93 into flutter:main Jul 7, 2022
@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Jul 7, 2022

Thanks again for the contribution @blaugold !

@blaugold
Copy link
Contributor Author

blaugold commented Jul 7, 2022

Thanks to all the reviewers helping me land this!

@blaugold
Copy link
Contributor Author

blaugold commented Jul 7, 2022

@stuartmorgan Just letting you know that the linked issues did not get closed by merging this.

@jmagman jmagman mentioned this pull request Jul 7, 2022
Closed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 8, 2022
Merged
@blaugold
Copy link
Contributor Author

blaugold commented Jul 12, 2022

@jmagman flutter/flutter#96391 should have been closed as well when this PR was merged, but is still open.

@ditman
Copy link
Member

ditman commented Jul 12, 2022

Thanks for the contribution @blaugold!!

About the auto-close, I created this: flutter/flutter#106211, but I'm not sure if this is a flutter repo issue, or a github issue (or a mix of both!)

@wannabehero wannabehero mentioned this pull request Nov 11, 2022
Merged
11 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

5 more reviewers

@jmagman jmagman jmagman left review comments

@camsim99 camsim99 camsim99 approved these changes

@stuartmorgan-g stuartmorgan-g stuartmorgan-g approved these changes

@ditman ditman ditman approved these changes

@cyanglaz cyanglaz cyanglaz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App p: google_sign_in

Projects

None yet

Milestone

No milestone

8 participants

@blaugold @radiKal07 @ditman @stuartmorgan-g @jmagman @DominikKunadt @cyanglaz @camsim99