-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Google SignIn without SDK #20128
Conversation
73ee4d5
to
f5f311e
Compare
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
e706ae7
to
98a6b7f
Compare
let googleLogingWithoutSDK: Bool = { | ||
switch BuildConfiguration.current { | ||
case .appStore: | ||
// Rely on the remote flag in production | ||
return RemoteFeatureFlagStore().value(for: FeatureFlag.sdkLessGoogleSignIn) | ||
case _: | ||
// Do whatever is hardcoded or overridden on device in non-production builds | ||
return FeatureFlag.sdkLessGoogleSignIn.enabled | ||
} | ||
}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hassaanelgarem @momo-ozawa you've been working on feature flags right?
Is this a reasonable approach? I was surprised by having to use a dedicated store to read remote values instead of accessing them directly from FeatureFlag.<case>.enabled
so I'm not sure whether I'm using it right.
I considered moving this logic into the enabled
implementation, but realized it would have created an infinite loop in case the value was not found remotely 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was surprised by having to use a dedicated store to read remote values instead of accessing them directly from FeatureFlag..enabled
Yes, using remote feature flags is really unintuitive at the moment, but coincidently, this is something I'm working on fixing during HACK week (we're having a late HACK week within our team cause we were on release rotation during the original week). Ref: pbArwn-5Uc-p2#comment-7452
Is this a reasonable approach?
Yes, this looks reasonable. However, you could also ditch the whole switch and only depend on RemoteFeatureFlagStore.value(for:)
. Currently, value(for:)
returns the overridden value if it exists. If not, it returns the remote value if it exists. If not, it returns the default (hardcoded) value. If you've added the switch just to facilitate testing, you can consider removing it. If it serves a different purpose, then this looks good 👍
FYI: There's a known issue where if you have a remote flag that is remotely set to true
, you can't override it to return false
. This is also something I'm working on fixing now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback @hassaanelgarem 🙇♂️
However, you could also ditch the whole switch and only depend on
RemoteFeatureFlagStore.value(for:)
. Currently,value(for:)
returns the overridden value if it exists. If not, it returns the remote value if it exists. If not, it returns the default (hardcoded) value.
🤔
Interesting. Maybe there's a way to move those build-time configuration at the feature toggle level then? I'd like to force it to true
for testing within Automattic, but start as false
in production.
I'll look into it and update if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into it and update if necessary.
For the record, I've rebased and adopted the new syntax vai e91ca3c
596bffa
to
fc71c9a
Compare
8c0ba7e
to
940e399
Compare
switch service { | ||
case .google(let user): | ||
fullName = user.profile?.name ?? String() | ||
email = user.profile?.email ?? String() | ||
case .apple(let user): | ||
fullName = user.fullName | ||
email = user.email | ||
} | ||
fullName = service.user.fullName | ||
email = service.user.email |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the WordPressAuthenticator level, both cases now have the same associated type (because we no longer rely on Google's GIDGoogleUser
. user
is a computed var on SocialService
that hides away that switch
and unwrapping logic. See wordpress-mobile/WordPressAuthenticator-iOS#764
Of course, it didn't work out of the box 🙃 But the issue was not with the APIs but with the port. See wordpress-mobile/WordPressAuthenticator-iOS#764
I since discovered that we already have extensive events tracked. There is little additional value in adding more to track whether we are using the Google SDK compared to the amount of code we'd need to write. |
940e399
to
0644db7
Compare
Generated by 🚫 dangerJS |
Podfile
Outdated
pod 'WordPressAuthenticator', '~> 5.6-beta' | ||
# pod 'WordPressAuthenticator', git: 'https://github.com/wordpress-mobile/WordPressAuthenticator-iOS.git', branch: 'trunk' | ||
# pod 'WordPressAuthenticator', '~> 5.6-beta' | ||
pod 'WordPressAuthenticator', git: 'https://github.com/wordpress-mobile/WordPressAuthenticator-iOS.git', branch: 'mokagio/social-service-without-google' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll point back to trunk
once the PR for this branch (wordpress-mobile/WordPressAuthenticator-iOS#764) is merged and before merging this.
Just a note to say that I'll start the 22.1 code freeze soon but I'll leave this PR in 22.1 aiming to ship it with a followup beta. The reason I want to do this is to avoid delaying the testing of the Google SignIn without SDK in the real world for other two weeks. |
c4c6bfb
to
144b74a
Compare
case .appStore: | ||
// Rely on the remote flag in production | ||
return RemoteFeatureFlag.sdkLessGoogleSignIn.enabled(using: remoteFeaturesStore) | ||
case _: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it makes sense that the _
syntax would compile here. But I think default
is more suitable for "all other cases"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But look at how homogeneous it looks with only case
. 😄
I never dug this up, but I don't see any mention of case _
in the Swift book. Also taking into account the more relatively recent @unknown default
syntax, it makes sense to prefer default
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the usage of _
here falls into the Wildcard Pattern
144b74a
to
3fee26b
Compare
While `case _` works, `default` is the recommended syntax. See conversation at #20128 (comment)
Ouch! I should have updated the target branch 🤦♂️ This was supposed to land on The damage is done now. While I'd love to get real world feedback ASAP, I don't think it's worth jumping through hoops to cherry pick these changes into the release branch. 😞 |
There were conflicts on `Podfile` and `Podfile.lock` because `release/22.1` and `trunk` both changed the Gutenberg version. As usual, I resolved them with `git checkout --theirs`, keeping the value from `trunk` under the assumption that the latest version of Gutenberg, 1.93.0-alpha2, is what `trunk` needs to work and that if the 1.92.1 hotfix hasn't already been integrated in the latest release, it will be soon and the Gutenberg team will followup with a new version update on `trunk`. There was also a conflict on the `RELEASE-NOTES.txt` that GitHub picked up with its more refined conflicts engine and that auto-merge didn't address properly but that I manually fixed. That was due to `release/22.1` adding new release note entries – which will not make it into the App Store release notes – in the 22.1 section and `trunk` having #20128 editing that section, too. #20128 was meant to land on `release/22.1` but I forgot to update the base branch after starting the code freeze 🤦♂️😭 ``` <<<<<<< release/22.1 -- Incoming Change * [**] Add a "Personalize Home Tab" button to the bottom of the Home tab that allows changing cards visibility. [#20369] * [*] [Reader] Fix an issue that was causing the app to crash when tapping the More or Share buttons in Reader Detail screen. [#20490] * [*] Block editor: Avoid empty Gallery block error [WordPress/gutenberg#49557] ======= * [**] [internal] Refactored Google SignIn implementation to not use the Google SDK [#20128] >>>>>>> trunk -- Current Change ```
There were conflicts on `Podfile` and `Podfile.lock` because `release/22.1` and `trunk` both changed the Gutenberg version. As usual, I resolved them with `git checkout --theirs`, keeping the value from `trunk` under the assumption that the latest version of Gutenberg, 1.93.0-alpha2, is what `trunk` needs to work and that if the 1.92.1 hotfix hasn't already been integrated in the latest release, it will be soon and the Gutenberg team will followup with a new version update on `trunk`. There was also a conflict on the `RELEASE-NOTES.txt` that GitHub picked up with its more refined conflicts engine and that auto-merge didn't address properly but that I manually fixed. That was due to `release/22.1` adding new release note entries – which will not make it into the App Store release notes – in the 22.1 section and `trunk` having #20128 editing that section, too. #20128 was meant to land on `release/22.1` but I forgot to update the base branch after starting the code freeze 🤦♂️😭 Also, the entry about the personalize home tab had been removed on `trunk` via ec8a377. The release notes editor was notified about it, #20483 (review) ``` <<<<<<< release/22.1 -- Incoming Change * [**] Add a "Personalize Home Tab" button to the bottom of the Home tab that allows changing cards visibility. [#20369] * [*] [Reader] Fix an issue that was causing the app to crash when tapping the More or Share buttons in Reader Detail screen. [#20490] * [*] Block editor: Avoid empty Gallery block error [WordPress/gutenberg#49557] ======= * [**] [internal] Refactored Google SignIn implementation to not use the Google SDK [#20128] >>>>>>> trunk -- Current Change ```
Relies on the changes in WordPress Authenticator 5.6.0 to run Google SignIn without the Google SDK, plus the fixes for issues found after integrating, see wordpress-mobile/WordPressAuthenticator-iOS#764 and wordpress-mobile/WordPressAuthenticator-iOS#762. See also wordpress-mobile/WordPressAuthenticator-iOS#741
The behavior is off by default for App Store build, on otherwise. That is, App Store builds will still use the SDK, but all other builds won't. This should give us time to test the new implementation internally and see if we run against hard edges.
Testing
Given the "on internally" configuration, one can verify that Google SignIn doesn't use the SDK by adding a breakpoint in
GoogleAuthenticator.swift
(a file from the WordPressAuthenticator pod) line 190 (or wherever you prefer in that method or other methods in the Google authentication chain) and attempting to login with Google:2023-03-21.13-24-29.2023-03-21.13_25_36.mp4
Repeat by either changing the hardcoded value or by enabling the flag from the in-app debug view and restarting the app (from Xcode, given we're interested in breakpoints) and observe the breakpoint being hit.
Regression Notes
Next steps
RELEASE-NOTES.txt
if necessary. N.A.