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

Development -> main #57

Open
wants to merge 53 commits into
base: main
Choose a base branch
from
Open

Development -> main #57

wants to merge 53 commits into from

Conversation

WcaleNieWolny
Copy link
Contributor

No description provided.

accessToken: AccessToken | null;
idToken: string | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

this can't be removed, it's currently in used by users, new offline system shouldn't modify current one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not what you and I agreed on.
image

This entire PR is not JUST about offline mode. It also introduces the "fixes" that unify the experience between all 3 platforms. As you probably remember well, it's not possible to provide idToken on the web, so we BOTH decided that the best course of action would be to just remove it entirely.

This PR is meant to be breaking. It should receive a new major release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, bit right now idtoken is used by some users who migrate and use only native, and it’s also common in all auth we have so. I believe maybe we can make it as a config option to use the new system ? And in major it will be by default , or with option to disable it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the problem is going to be explaining to users that idtoken is native only. I am like 99% sure that you will get users complaining about idtoken not working on the web and explaining to people that it's an unsolvable issue is not great. I sort of think we should be consistent in this. This development branch has been up for a couple of weeks at this point and I have already instructed users how to migrate AWAY from idtoken.

Personally, I do not believe that it is that big of a deal. If you make it a breaking change with a new major, then it's sort of normal that a migration might be necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that verifying idtoken server-side is what Google docs recommend: https://developers.google.com/identity/gsi/web/guides/verify-google-id-token#node.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am well aware, however as I already said, google does not provide a modern SDK to get the idtoken on the web.

To be more specific, the provide a way to render a Sign in with google button
image
This button does in fact return idtoken.

However, this requires the end user to make changes to their UI for the web version. I do not believe that this is acceptable, and as such idtoken will be removed from ALL the platforms, including iOS and Android.

Providing a unified user experience between all the platforms is the highest priority of this plugin.
I will reconsider adding idtoken when Google updates theirs SDKs

@@ -5,8 +5,12 @@

public interface SocialProvider {
void login(PluginCall call, JSONObject config);

Copy link
Contributor

Choose a reason for hiding this comment

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

this diff shouldn't be present

Copy link

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@types/[email protected] None 0 32.6 kB types
npm/[email protected] None 0 22.7 MB typescript-bot

🚮 Removed packages: npm/[email protected]

View full report↗︎

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

Successfully merging this pull request may close these issues.

3 participants