Code review changes

[rishabhpoddar]

Changes done in this PR

• Modifications to .npmignore
• Remove the ^ from all import statements. We do not want to risk the functioning of our package on someone else making an error while releasing their code. In reference to Peer dependency versions #14
• Added index.ts, index.js, index.d.ts at the root of this project so that importing via supertokens-auth-react is possible.
• Corrected the exporting of modules from the index files used to import from this project. You were exporting just the class, and not the methods.

Commit

• Removed logoFullURL from appInfo as per this comment. The tests for this are still there in this commit, but are removed in the next one.
• Moved EmailPasswordConfig into its own types.ts inside the recipe/emailpassword folder, since this type is specific to this recipe.

Commit

• This is a large amount of change, I can take you through it over a call
• No need to use removePendingSlashFromPath when we have normaliseURLPathOrThrowError
• Created NormalisedURLDomain and NormalisedURLPath so that we enforce normalisation whenever dealing with paths / domains. I intend to use these on the backend SDK as well.
• Makes SuperTokensUrl use NormalisedURLPath.
  • Moved the respective normalising functions from utils to these files so that other files need to use these classes and not the functions directly.
  • There were a few inconsistencies in how you handled paths, sometimes, you had a trailing / and sometimes you didn't. Using this class sorted those issues out.
• Changes all types to use NormalisedURLPath and NormalisedURLDomain (except for user input types)
• Changed other files and tests accordingly
• Adds mocha to devDependencies so that unit tests can be run
• The existing npm run build command was not generating .d.ts files. I changed the npm run build command to make typescript generate .d.ts files, and make .js files using babel.
• Corrected CONTRIBUTING.md to say to also do npm i -d in react-test-app during project setup. Otherwise npm run test doesn't work.
• Changed RecipeModule base class to convert the incoming features map to be normalised paths.

Commit

• Saves appInfo in the base class of recipes. This is done by converting the function () => RecipeModule to (appInfo: AppInfo) => RecipeModule
• Due to the above, we can now store the full path in the features map in recipes. This also meant that we could get rid of pathWithoutWebsiteBasePath and its associated function (in the class SuperTokensUrl)

Commit

• I changed getInstanceIfDefined to getInstanceOrThrowError since it's more accurate.

[rishabhpoddar]

Changes not done, but to discuss

• Can we move testApp.sh into a folder like test or rename hooks to script and move it in there?
• I noticed that there is an emotion-server. Do you know what that does? It's quite strange.
• Can we move react-test-app into the test folder? Since that is also about testing.
• When I first ran npm run test, a lof of the tests threw CONNECTION REFUSED for localhost:3031 error.. then I ran testApp.sh --wait, and then I reran npm run test, but then it said react not found. Then I did npm i -d, and then reran npm run test. Then it worked. And then I stopped the testApp, and then npm run test worked again.. Not sure what's happening here..
• The name SuperTokensUrl is not good.
• Maybe I am misunderstanding the routing code, but it does not seem to be returning the abstract component that decides which feature to show based on the rId of the current page. I understand that it returns a list of Routes pointing to the feature components for each unique route.
• The getRoutingComponent function in recipeModule returns the feature component, not a component that decides which feature to render based on the rId.
• Ideally would like to have the index.ts file in the emailpassword recipe as a wrapper, similar to how it's done for supertokens.
• I'm not sure why this happens, but many times, I run tests, it fails. Then i remove the build dir, and rerun tests, they pass. There is no code diff between the two as get status in both cases is clean. Strange...
• We should not have build in npm run test because then we may be testing something that's different to what's being shipped?

[kant01ne]

Added index.ts, index.js, index.d.ts at the root of this project so that importing via supertokens-auth-react is possible.

No need for those since the main in package.json points to lib/build/index.js

Corrected CONTRIBUTING.md to say to also do npm i -d in react-test-app during project setup. Otherwise npm run test doesn't work.

and

When I first ran npm run test, a lof of the tests threw CONNECTION REFUSED for localhost:3031 error.. then I ran testApp.sh --wait, and then I reran npm run test, but then it said react not found. Then I did npm i -d, and then reran npm run test. Then it worked. And then I stopped the testApp, and then npm run test worked again.. Not sure what's happening here..

I wrote a README specifically for the test app (in my next branch), explaining all about installing npm i -d and why sometimes there are issues with React react-dom-router conflicts and how to solve.

The setup is not ideal at the moment though. Basically the dev dependencies from the sueprtokens-auth-react are clashing with the dependencies from the react test app package. That will not happen in production or in CI but this is something that needs to be adressed locally too.
I could simply remove react/react-router-dom from react-test-app and it would use sueprtokens-auth-react packages, with no conflicts. But we might be missing on other problems' so I'd rather not do that.

I noticed that there is an emotion-server. Do you know what that does? It's quite strange.

Maybe I am misunderstanding the routing code, but it does not seem to be returning the abstract component that decides which feature to show based on the rId of the current page. I understand that it returns a list of Routes pointing to the feature components for each unique route.
The getRoutingComponent function in recipeModule returns the feature component, not a component that decides which feature to render based on the rId.

I think you got it right, there is a difference between getRoutingComponent algorithm (which returns the right component directly) and the algorithm for react-router-dom which returns the Route containing a component (which handles the route too but based on query params).

We should not have build in npm run test because then we may be testing something that's different to what's being shipped?

Oh interesting, i didn't think of that. I found it more efficient for development purposes but I you are right.

I noticed that there is an emotion-server. Do you know what that does? It's quite strange.

emotion-server is used for server-side-rendering with emotion. ReactShadow leverages this package to write inside the DOM of the shadow dom so this is needed.

Question

• Why using the name getAsStringDangerous when the return value is normalised?

The code looks good! Thanks for the PR.
Should I squash and merge to my PR or should I squash and merge mine to 0.0 first?

[kant01ne]

btw, some of the changes you made (Logo, types to emailpassword) I have made in my next branch, so I should have pushed everything to that branch anyway. Now is time to resolve some conflicts 😄

[rishabhpoddar]

Why using the name getAsStringDangerous when the return value is normalised?

Because when you get the string directly, you need to realise that it's up to you to make sure any manipulation of that string should also be normalised. Ideally any checks, manipulation etc.. should be done by the class itself to guarantee that paths / domains are always normalised within the program.

Should I squash and merge to my PR or should I squash and merge mine to 0.0 first?

To your PR.

The code looks good! Thanks for the PR.

Thanks!

[kant01ne]

I started to have conflicts after squashing my commits into one.
Closing this PR as I've cherry picked your commits into recipe-module-manager directly