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

[WIP] fix: utilize 'error-causes' to create errors for 'signIn' and 'signUp' SDK functions #52

Closed
wants to merge 4 commits into from

Conversation

oliver-day
Copy link
Contributor

@oliver-day oliver-day commented Nov 14, 2022

Description

Questions

  1. Should I add error-causes to greenruhm-web and have all possible errors generated during sign up with error-causes (If so should we come up with a convention for error codes)?
  2. Should a 500 response from the greenruhm-web api be handled with this implementation?

NOTE: I have included an UserRejectedConsentToShareEmail error so that all errors generated in the existing sign in and sign up flows are created with error-causes. In an upcoming PR, we will move away from using magic.connect.requestUserInfo() and this error will no longer be necessary so it can be removed at that point in time.

TODO

@oliver-day oliver-day self-assigned this Nov 14, 2022
@oliver-day oliver-day linked an issue Nov 14, 2022 that may be closed by this pull request
3 tasks
@oliver-day oliver-day changed the title fix: utilize 'error-causes' to create errors for 'signIn' SDK function [WIP] fix: utilize 'error-causes' to create errors for 'signIn' and 'signUp' SDK functions Nov 14, 2022
@ericelliott
Copy link
Contributor

ericelliott commented Nov 15, 2022

Should I add error-causes to greenruhm-web and have all possible errors generated during sign up with error-causes (If so should we come up with a convention for error codes)?

You mean, first create errors with named causes in Greenruhm Web so that you can translate them to caused errors here based on the responses from Greenruhm Web? If it saves you time on rework, why not?

Should a 500 response from the greenruhm-web api be handled with this implementation?

You should be able to handle that with the default error response. Error causes has an unexpected error case built-in:

const UnexpectedError = {
  name: "UnexpectedError",
  message: "An unexpected error was thrown",
};

Copy link
Contributor

@ericelliott ericelliott left a comment

Choose a reason for hiding this comment

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

It looks like we removed the error handling logic for Magic errors - I assume we're moving that error logic elsewhere. Is this correct? Where did it move to?

@oliver-day
Copy link
Contributor Author

It looks like we removed the error handling logic for Magic errors - I assume we're moving that error logic elsewhere. Is this correct? Where did it move to?

Within src/features/user/sign-up.js and src/features/user/sign-in.js

@ericelliott
Copy link
Contributor

ericelliott commented Nov 19, 2022

won't I need to merge the named signUpErrors I created in this #52 into the main branch of connect before I will be able to reference them in greenruhm-web?

Yes.

If so the path forward would be:

  1. Merge the errors you need to display in connect so that you can import them into Greenruhm-web.

Those are the ones with http status codes like 400 and 500.

One step at a time.

@oliver-day
Copy link
Contributor Author

oliver-day commented Dec 1, 2022

Closing this PR as I have made an updated one with the fix/sdk-functions-error-handling-1 branch. This was done to avoid having to resolve merge conflicts as a result of later work where connect was updated to use CJS modules and additional changes to named error causes.

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.

2 participants