capacitor-community / apple-sign-in

Sign in with Apple Support
MIT License
136 stars 59 forks source link

Add web support #2

Closed priyankpat closed 3 years ago

priyankpat commented 4 years ago

Address #1 Added web support

priyankpat commented 4 years ago

When I invoke Authorize(), window.AppleID returns undefined but console shows the AppleID object present inside window. Object.keys(window) does not contain AppleID key. Looking into it.

Taken care of in db3a8fa Added small timeout (100ms)

chrisweight commented 4 years ago

@mlynch - who needs to review this to be able to approve/merge/request changes etc?

priyankpat commented 4 years ago

@chrisweight Max will be partially maintaining this repo and is looking for help to maintain it. Maybe you should take over since you wrote a good chunk of code in this PR

chrisweight commented 4 years ago

@chrisweight Max will be partially maintaining this repo and is looking for help to maintain it. Maybe you should take over since you wrote a good chunk of code in this PR

I have got exactly zero time, sadly!

mlynch commented 4 years ago

Can someone clarify what SignInResponse and ResponseSignInWithApplePlugin are? They conflict yet have different structures

chrisweight commented 4 years ago

ResponseSignInWithApplePlugin

@mlynch - I can't find a reference to this on this PR?

mlynch commented 4 years ago

It's in master and is conflicting and I'm not sure how to resolve:

https://github.com/capacitor-community/apple-sign-in/blob/master/src/definitions.ts#L11

chrisweight commented 4 years ago

It's in master and is conflicting and I'm not sure how to resolve:

https://github.com/capacitor-community/apple-sign-in/blob/master/src/definitions.ts#L11

Ok, so the ResponseSignInWithApplePlugin is obviously what's being returned from the iOS plugin, and the SignInResponse is what's coming back from the web plugin, the latter matching the JSON response structure that Apple returns.

So, two choices I guess: Clarify the naming of each top-level response interface, e.g: ResponseiOS and ResponseWeb, then allow for both types in the Authorize method call, i.e: Authorize(): Promise<{ response: ResponseiOS | ResponseWeb }>;

Or define a common structure that can be used to mulch responses into the same form, regardless of platform.

@mlynch, @priyankpat - thoughts?

chrisweight commented 4 years ago

Also: the iOS plugin will need a noop for the Init method, right?

priyankpat commented 4 years ago

@mlynch @chrisweight I fixed the conflicts. Aggregated the return types. Should I make them optional?

export interface ResponseSignInWithApplePlugin {
  response: {
      ...
  };
  user: User;
  authorization: Authorization;
}

Maybe rename the user and authorization to webUser and webAuthorization?

Don't think noop is required.

priyankpat commented 4 years ago

@mlynch I can merge this PR if you are good with the changes.

chrisweight commented 3 years ago

@mlynch I can merge this PR if you are good with the changes.

bump on this @mlynch ?

chrisweight commented 3 years ago

@mlynch I can merge this PR if you are good with the changes.

There's been no response on this - you should probably just merge the PR @priyankpat and then addresses any issues as separate fixes?

chrisweight commented 3 years ago

tumbleweed ... anyone?

mlynch commented 3 years ago

Can someone resolve the conflict? Then can merge it

seanaye commented 3 years ago

Is there any updates on this? Is there a documented way of this working for web? I have installed this branch but SignInWithApple.authorize() rejects with error SignInWithApple does not have web implementation.

chrisweight commented 3 years ago

Is there any updates on this? Is there a documented way of this working for web? I have installed this branch but SignInWithApple.authorize() rejects with error SignInWithApple does not have web implementation.

@seanaye - it looks like @epicshaggy added web support in a separate PR and that was merged already.

@mlynch / @epicshaggy - might as well close this PR given #18 is now in master?

mesqueeb commented 3 years ago

The status of web support for this is unclear. From the readme it sounds like there is no support yet. Should this PR stay open or be closed?

@maxlynch @pilit

Are you two the official maintainers of this plugin? Any ideas?

seanaye commented 3 years ago

@mesqueeb it appears that this package is not updated frequently. I am not a maintainer but I have a fork that is working in production, both on web and capacitor ios

EDIT: I will update my readme so it is more clear

mesqueeb commented 3 years ago

@seanaye is it possible that your package becomes the official community plugin? @dwieeb do you have any ideas how we can make @seanaye's package the official one ?

seanaye commented 3 years ago

@mesqueeb It is likely better if I just become a maintainer. The official package should not be moved out of the @capacitor-community namespace. I also don't have the knowledge to maintain this package on my own (I'm not a swift developer).

Here is the my fork, I've updated the readme to reflect how I'm using it. https://github.com/seanaye/apple-sign-in

epicshaggy commented 3 years ago

@seanaye @mesqueeb Hi guys, web support was added in #18. The package is live on npm, web implementation is the same as for iOS. Checkout the readme, the Usage section was updated with the addition of web support. If you're still getting errors do let me know. 😃