Code review for branch feature-email-password

[rishabhpoddar]

Issues from past PR that are still there:

• Are the issues related to testing solved?
  • lots of the tests threw CONNECTION REFUSED
• Is the function SuperTokensRouteWithRecipeId in superTokensRoute.tsx equal to the render function of that component?

New issues / questions

• In index.ts of the recipe, you also need to export all the functions inside the Wrapper class, so that they can be accessed via import * as ___ from "..."
• In index.ts of the recipe, you also need to export all the things that are outside the Wrapper class so that they can be accssed via import ___ from "..."
• Please check your code for if statements without {}. I found one instance in getPathsToComponentWithRecipeIdMap in superTokens.tsx. Also, please use explicit comparison.. Does if ([]) equal to if (false)?
• This is a small issue, but there are some cases in which you call a static function from a non static function. The static function then gets the instance and calls the relevant non static function. Example is in getMatchingComponentForRouteAndRecipeId in superTokens.tsx
• getMatchingComponentForRouteAndRecipeId should take path as NormalisedURLPath. Not as a string. You want to convert this class to a string as late as possible, and only if absolutely necessary.
• getMatchingComponentForRouteAndRecipeId is written in a way that if a rId is provided in the URL, but that doesn't match any recipe, then we will treat it as if rId is not there is not there in the URL. Is this expected?
• In canHandleRoute, I think it's better to call the getRoutingComponent and check if that is undefined or not. This way, it's guaranteed that if we change the routing algo, then canHandleRoute will change accordingly.
• We should abstract away new NormalisedURLPath(window.location.pathname) into a function in utils. This way, if it turns out later that window.location.pathname is not correct, then we can easily change it. It also gives the advantage that we do not use window.location.pathname without first normalising it anywhere.
• In httpRequest.ts, if config is undefined, and we do ...config, will that throw an error?
• In httpRequest.ts, no need for supertokens-auth-react-version
• Can we move some of the constants, styles and components specific to recipes into their folder?
• Do we need canHandleRoute and getRoutingComponent in recipeModule.ts?
• If we are allowing users to change the background for the default routes, then where will that go? If we allow them to do it via style in the init, what if they give a different colour for sign up and sign in?
• For handling signInAndUpFeature object in the init call, I suggest you create a function like validateAndNormaliseSignInAndUpFeatureConfig which takes what the user gave and returns the fully normalised config back which you can then propagate to the features. So this function will:
  • normalise all the form fields, and insert default ones if necessary
  • merge the styles?
  • Set onSuccessRedirectURL to / if the user has not provided one
  • etc..
• For handling disabledDefaultImplementation, you are using the user's provided config before actually normalising it first. The first thing we want to do in the recipe is normalise all of the user's input, and add all defaults and then use that object.
• Can we convert SignInAndUp to a normal class that extends React? Having functions inside a function, not being able to control things like shouldComponentUpdate etc.. are all restrictive. I prefer to use functional component only if they are not complex.
• validateFormOrThrow in utils.ts does not throw any error.
• Like all other modules, even for SignInAndUp, I prefer if we first normalise (add defaults) the config / props given by the user and store the normalised version of that as an instance variable. So this way, when you are calling the signUp API, you do not have to check if props.signUpApi === undefined. You can just do this.config.signUpApi(..).
• In the components folder inside emailpassword, we should have a themeComponent folder
• signUpApi and signInApi in emailPassword.ts do not take care of error handling?
• Error handling for API calls needs to be done assuming the user can use axios or fetch.
• In SignInAndUp, signInAPI should not change state.. it should only return the response of the API call in a normalised way. We need to segregate API call layer from state layer of a component.
• Use useEffect carefully
• The API call functions take and return a JSON. If the status is >= 300, they throw an error. So we don't really care if the user uses axios or fetch. However, we have to follow this same semantic in our default implementations
• Need to figure out if the SuperTokensRouteWithRecipeId runs every render or every route change?
• Add the fdi-version header to requests as per Add fdi-version header to requests frontend-driver-interface#2.

[kant01ne]

IN PROGRESS

Are the issues related to testing solved?
lots of the tests threw CONNECTION REFUSED

You'll need to run cd test/react-test-app && npm install before since the folder has moved and there are new libs added.

Is the function SuperTokensRouteWithRecipeId in superTokensRoute.tsx equal to the render function of that component?

Need to figure out if the SuperTokensRouteWithRecipeId runs every render or every route change?

This happens only when the specific route is clicked. So most likely only once per navigation.
I moved the component to getSueprTokensRoutesForReactRouterDom anyway because it was crashing when react-router-dom was not present which was the reason why we had this try catch in the first place.

Please check your code for if statements without {}. I found one instance in getPathsToComponentWithRecipeIdMap in superTokens.tsx.

Fixed and added https://eslint.org/docs/rules/curly to eslint prevent this from happening again.

Does if ([]) equal to if (false)?

Added https://eslint.org/docs/2.0.0/rules/no-implicit-coercion to eslint to prevent this from happening again.

getMatchingComponentForRouteAndRecipeId is written in a way that if a rId is provided in the URL, but that doesn't match any recipe, then we will treat it as if rId is not there is not there in the URL. Is this expected?

Yes that's what we decided in our previous discussion around routing. If no rid is provided, or if rid is unknown we want to use the first recipe feature that matches.

In httpRequest.ts, if config is undefined, and we do ...config, will that throw an error?

config is now required.

validateFormOrThrow in utils.ts does not throw any error.

It actually does throw if length is different as per our conversation.

In the components folder inside emailpassword, we should have a themeComponent folder

After our discussion we decided to structure it that way:

| recipe/
|--emailpassword/
|----index.ts
|----types.ts
|----(...)
|----assets/
|----components/
|------library/
|--------input.ts
|--------(...)
|------signInAndUp/
|--------index.ts
|--------themes/
|----------default/
|------------index.ts
|------------SignInTheme.ts
|------------SignUpTheme.ts
|----------darkTheme/
|----------blueTheme/ 
etc...

[kant01ne]

Fix #21