Skip to content
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

Fix Google Login button disappearing issue #3197

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

GspikeHalo
Copy link
Collaborator

@GspikeHalo GspikeHalo commented Jan 7, 2025

Purpose:

The current Google login button occasionally disappears. The reason is that the Google script is loaded statically, meaning it is automatically loaded via the <script> tag when the page loads. This approach has a potential issue: if we want to use the onGoogleLibraryLoad callback function to confirm that the script has successfully loaded, we must ensure that the callback is defined before the script finishes loading. Otherwise, when the script completes loading, Google will check whether onGoogleLibraryLoad is defined. If it is not defined, the script load notification will be missed, and the Google login initialization logic will not execute.
fix #3155

Changes:

  1. To reduce the maintenance cost caused by frequent updates to the Google official API, we chose to implement Google login using the third-party library angularx-social-login instead of directly relying on the Google official API.
  2. Added the dependency @abacritt/angularx-social-login (version 2.1.0). Please use yarn install to install it.
  3. Additionally, a test file for the Google Login component has been added to verify whether the component initializes correctly after the script is successfully loaded.

Demos:

Before:

before.mp4

After:

after.mp4

@GspikeHalo GspikeHalo requested a review from kunwp1 January 7, 2025 02:18
@GspikeHalo GspikeHalo self-assigned this Jan 7, 2025
Copy link
Collaborator

@kunwp1 kunwp1 left a comment

Choose a reason for hiding this comment

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

It would be better to set up a Promise to guarantee that the window.onGoogleLibraryLoad function is defined before the Google script attempts to call it. In the current implementation, having two separate window.onGoogleLibraryLoad function calls within the same file feels inconsistent.

@GspikeHalo
Copy link
Collaborator Author

It would be better to set up a Promise to guarantee that the window.onGoogleLibraryLoad function is defined before the Google script attempts to call it. In the current implementation, having two separate window.onGoogleLibraryLoad function calls within the same file feels inconsistent.

I made some mistakes in my previous description. Currently, the Google script is loaded statically, meaning it is automatically loaded via the <script> tag when the page loads. This approach has a potential issue: if we want to use the onGoogleLibraryLoad callback function to confirm that the script has successfully loaded, we must ensure that the callback is defined before the script finishes loading. Otherwise, when the script completes loading, Google will check whether onGoogleLibraryLoad is defined. If it is not defined, the script load notification will be missed, and the Google login initialization logic will not execute.

To address this timing issue, I decided to stop relying on the onGoogleLibraryLoad callback. Instead, I will directly check the global variable window.google to determine if the script has finished loading. This approach effectively avoids problems caused by defining onGoogleLibraryLoad after the script has already loaded. I will also update the PR description accordingly.

Copy link
Collaborator

@kunwp1 kunwp1 left a comment

Choose a reason for hiding this comment

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

Looks better than the previous version. Please add a comment that timeout is in milliseconds and also add a unit test for the following scenario:

"should initialize Google login button when script is loaded"

@kunwp1 kunwp1 requested a review from aglinxinyuan January 9, 2025 19:10
@kunwp1
Copy link
Collaborator

kunwp1 commented Jan 9, 2025

@aglinxinyuan Adding you to be a second reviewer for this PR since you are familiar with the Google Login feature.

@aglinxinyuan
Copy link
Collaborator

The code itself seems fine to me, but maintaining such a simple function might require significant effort on our end. For instance, we would need to monitor any API changes from Google's side and ensure it's always up to date. It might be worth considering using a third-party library instead.
Maybe try this: https://www.npmjs.com/package/@abacritt/angularx-social-login

@GspikeHalo GspikeHalo marked this pull request as draft January 16, 2025 00:11
@GspikeHalo GspikeHalo marked this pull request as ready for review January 16, 2025 04:25
@GspikeHalo GspikeHalo requested a review from kunwp1 January 16, 2025 04:25
Copy link
Collaborator

@aglinxinyuan aglinxinyuan left a comment

Choose a reason for hiding this comment

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

LGTM!

{
id: GoogleLoginProvider.PROVIDER_ID,
provider: new GoogleLoginProvider(response?.clientId || "", {
oneTapEnabled: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any reason to disable oneTap?

Copy link
Collaborator Author

@GspikeHalo GspikeHalo Jan 17, 2025

Choose a reason for hiding this comment

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

I found that when I enable One Tap, a message 'PR' appears in my console.
image
I noticed that if I disable it, the message does not appear. If One Tap is necessary, I can try to see if I can eliminate this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Login Button Disappears After Page Refresh
3 participants