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

Feature/state machine #90

Merged
merged 10 commits into from
Dec 20, 2023
Merged

Conversation

incorbador
Copy link
Contributor

The main idea of this PR is to remove all "hard" logic from @corabdo/react and to move it into the flowHandler.

Basic concept:

  • UI components emit events only and react to state updates from the flowHandler => they only contain state if this state is specifically needed for the framework to work (they no longer hold e.g. the email of a user)
  • the flowHandler holds a state-machine where every screen is a state (this is not completely new, previously flowHandler already was a bit like a state machine)
  • for each state we define events that can either update the current state, change to a new state (screen) or change the flow
  • @corbado/react connects to the flowHandler through FlowHandlerProvider

Advantages of this approach:

  • easier to maintain (logic + state is concentrated in a single place)
  • UI packages are easier to write (components just need to emit events, these events almost always correspond to user clicks so its quite easy for the developer to manage them)
  • errors can be handled where they are created (inside the flowManager)

Disadvantages:

  • quite a bit of refactoring
  • we are not as flexible anymore because UI components can only emit events now and no longer call functions as they want => this will constrain us, but in the end this can also be a good thing, because it forces us to be consistent in our logic between all frameworks

Future refactoring:

  • the definitions of flows have grown now => we can split them into one file per screen later, if we want

Additional changes:

  • Update the NonRecoverableError screen to match the Figma
  • Make sure that projectConfig is fetched before the component is shown (this is important to catch the error where a user would configure a wrong projectId)
  • Shorten the error keys in our translation files (the previous prefixes like "validation_error" or "server_error" are mainly driven by our backend and are probably hard to understand for users => I would opt for these shorter names)

@Aby-JS Aby-JS changed the base branch from main to error_handling_for_react December 19, 2023 09:11
packages/web-core/src/utils/errors/errors.ts Show resolved Hide resolved
packages/web-core/src/services/index.ts Outdated Show resolved Hide resolved
packages/web-core/src/services/ProjectService.ts Outdated Show resolved Hide resolved
packages/web-core/src/services/AuthService.ts Outdated Show resolved Hide resolved
packages/react/src/contexts/FlowHandlerProvider.tsx Outdated Show resolved Hide resolved
// Render the component if it exists, otherwise a fallback or null
return (
<ErrorBoundary
globalError={globalError}
isDevMode={isDevMode}
customerSupportEmail={customerSupportEmail}
>
{ScreenComponent ? <ScreenComponent /> : <EndComponent />}
{initialized ? ScreenComponent ? <ScreenComponent /> : <EndComponent /> : <LoadingScreen />}
Copy link
Contributor

Choose a reason for hiding this comment

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

My opinion for loader is, since Auth components are more like login/signup pages we should let the user handler their loaders and, if not that, then atleast make it customizable so that they can replace the loaders.
My suggested solution is to just let users handle the loader and emit an event when our component has been initialized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The easiest way for now is to have the Loading Spinner I think (for us its just adding this single component here and a bit less for the developer to take care of). If people ask for that functionality (controlling the loading by themselves), we can just as you say expose the initialised info).

packages/react/src/screens/LoadingScreen.tsx Outdated Show resolved Hide resolved
</div>
<div className='error-details'>
<div className='error-detail-row'>
<div className='error-detail-title'>Message</div>
<div className='error-detail-title'>:Message</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add translations for error boundary component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes valid point. For the NonRecoverableError component that we only show in DEV mode, English is fine I think. But for the generic error page, we should have a translation I think. I will add one.

packages/react/src/screens/login/EmailOTP.tsx Outdated Show resolved Hide resolved
@Aby-JS Aby-JS added the enhancement New feature or request label Dec 20, 2023
@Aby-JS Aby-JS linked an issue Dec 20, 2023 that may be closed by this pull request
6 tasks
@@ -100,6 +100,13 @@
"button_start": "Create passkey",
"button_skip": "Maybe later"
}
},
"unexpectedError": {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's follow a similar convention to:

"passkeySuccess": {
      "header": "Welcome!",
      "subheader": "Passkey created",
      "body_text1": "You can now confirm your identity using your ",
      "body_text2": "passkey or via email one time code",
      "body_text3": " when you log in.",
      "button": "Continue"
    }

then we can call this:

"unexpectedErrorSupport": {
      "header": "Something went wrong",
      "subheader": "We’re sorry that our service is currently not available.",
      "body_withCustomerSupport": "Please try again in a few moments and if the issue persists, please contact {{customerSupportEmail}}.",
      "body_noCustomerSupport": "Please try again in a few moments.",
      "button": "Refresh Page"
    }

@incorbador incorbador merged commit a0b4845 into error_handling_for_react Dec 20, 2023
1 check passed
@incorbador incorbador deleted the feature/state-machine branch December 20, 2023 08:18
incorbador added a commit that referenced this pull request Dec 20, 2023
* added error handling for invalid otp

* added new translation schema

* updated translation schema

* added error handling for initiate signup

* added error handling for initiate login

* added email otp error cases

* added error handling for all screens

* added error boundary for handling non recoverable errors

* fixed styling issues in error boundary

* updated german translation

* updated french translation

* Feature/state machine (#90)

* State machine for signup flows

* Update login flows

* Removed unused code, synced translations

* Added missing fields to NonRecoverabelError page

* Removed console.logs

* Temporarily deactivate the react-sdk-example

* PR feedback: Formatting, translations for generic error page, general feedback

* Fix hardcoded isDevMode in example

* Fixed react-sdk-example

* PR feedback: error translation structure

---------

Co-authored-by: Incorbador <[email protected]>

---------

Co-authored-by: Incorbador <[email protected]>
Co-authored-by: Incorbador <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Manage error handling in react package
2 participants