fix(singleton): allow oauth listener to be called with the latest config #13642
Conversation
israx
requested review from
ukhan-amazon,
haverchuck,
cshfang,
jimblanc,
HuiSF and
a team
as code owners
| // The setTimeout will only be called as long as an oAuthLister and oauth config | ||
| // are defined. The code executed by the time out will be only fired once. |
There was a problem hiding this comment.
This sounds like it only works with certain preconditions, e.g. assuming Amplify.configure() gets called back to back. While this trick seemed to be working, I'm hesitant to make this a solution...
With Amplify Gen2, amplify_outputs.json can be overridden from the backend.ts (reference). With Amplify Gen1, should be able to change the url in amplifyconfiguration.json as well, would be these be legit workarounds to solve what the PR is trying to solve?
There was a problem hiding this comment.
I have to echo this sentiment. When I looked at this PR this was my initial concern as well. Code after the configure call that may have previously executed after the this.oAuthListener call was made may now execute after the call due to it being pulled out of the current execution cycle. While I'm not certain that is practically concerning, it feels quite risky.
| // The setTimeout will only be called as long as an oAuthLister and oauth config | ||
| // are defined. The code executed by the time out will be only fired once. |
There was a problem hiding this comment.
I have to echo this sentiment. When I looked at this PR this was my initial concern as well. Code after the configure call that may have previously executed after the this.oAuthListener call was made may now execute after the call due to it being pulled out of the current execution cycle. While I'm not certain that is practically concerning, it feels quite risky.
| amplify: AmplifyClass, | ||
| config: AuthConfig['Cognito'], | ||
| oAuthListener: OAuthListener, |
There was a problem hiding this comment.
Making this pure would make sense if it were removed completely from the class, but since it is a private member of the class, this seems confusing. For example, it's not really clear to me why oAuthListener is passed separately from this. The clearListenerTimeout is then also tacked onto the instance but it's obfuscated here.
Description of changes
Amplify.configurewill calloAuthListenerand it will pass the Auth config. ThisoAuthListeneris responsable for completing an OAuth flow. However whenAmplify.configureis called more than 1 time, theoAuthListenerwill be called only with the first Auth config.This fix makes sure the
oAuthListeneris called only and only if a valid OAuthconfigandoAuthListenerare already defined. And ensures theoAuthListeneris called with the latest config of Amplify.Before
Amplify.configurewith config1 ->Amplify.configurewith config2 -> calloAuthListenerwith config1After
Amplify.configurewith config1 ->Amplify.configurewith config2 -> calloAuthListenerwith config2Issue #, if available
#12849
Description of how you validated changes
Checklist
yarn testpassesChecklist for repo maintainers
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.