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

Resolve signIn promise when sign in has completed #227

Merged
merged 4 commits into from
Dec 14, 2023

Conversation

johnnyd710
Copy link
Contributor

@johnnyd710 johnnyd710 commented Dec 13, 2023

tldr: Resolve the ElectronRendererAuthorization.signIn() promise when sign-in has fully completed, and reject on errors. Closes #226.

Changes

Previously, some sign-in errors (e.g., an internal server error on the token issuer server) were uncaught by ElectronRendererMain, and there wasn't a way for consumers of this package to catch it. The frontend who initiated the sign-in was left waiting for onAccessTokenChanged forever. Now, we resolve the promise when sign in has actually fully completed or reject the promise if there was an error during sign in, so that the frontend can show an error message.

Technically the API is the same, the types are the same, but the usage could vary slightly.

Example Usage of signIn()

Before

  const onSignInClick = async (): Promise<void> => {
    setSigningIn(true);
    if (IModelApp.authorizationClient instanceof ElectronRendererAuthorization) {
      await IModelApp.authorizationClient.signIn(); // resolves as soon as sign-in starts, with no way to catch errors
    }
  };

  useEffect(() => {
    (IModelApp.authorizationClient as ElectronRendererAuthorization)?.onAccessTokenChanged.addListener(
      (token: AccessToken | undefined) => {
        setAccessToken(token);
        setSigningIn(false);
      }
    );
  });

After

  const onSignInClick = async () => {
    setSigningIn(true);
    if (IModelApp.authorizationClient instanceof ElectronRendererAuthorization) {
      try {
        await IModelApp.authorizationClient.signIn(); // resolves after sign in is complete
      } catch (err) {
        LoggingClient.logException(err); // catch errors and do something
        setErrorMessage("Error while signing in, please try again.");
        setSigningIn(false);
      }
    }
  };

  useEffect(() => {
    (IModelApp.authorizationClient as ElectronRendererAuthorization)?.onAccessTokenChanged.addListener(
      (token: AccessToken | undefined) => {
        setAccessToken(token);
        setSigningIn(false);
      }
    );
  });

Copy link
Member

@aruniverse aruniverse left a comment

Choose a reason for hiding this comment

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

makes sense

@ben-polinsky
Copy link
Collaborator

LGTM, thanks.

@johnnyd710 johnnyd710 merged commit 833e9d6 into main Dec 14, 2023
13 checks passed
@johnnyd710 johnnyd710 deleted the john/catch-server-errors branch December 14, 2023 00:05
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.

ElectronMainAuthorization.signIn throws uncatchable error when tokenUrl server errors
3 participants