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

Add h3 framework #477

Open
wants to merge 6 commits into
base: 13.1
Choose a base branch
from

Conversation

ColinEspinas
Copy link

@ColinEspinas ColinEspinas commented Jan 16, 2023

Summary of change

This PR aims to add compatibility to h3 (and by extension Nitro) to Supertokens back-end.

Test Plan

So for now I am trying to get it to work with a simple nitro server with an empty "/" route by creating a plugin to init supertokens and adding the required middlewares to h3. This is still WIP and creating a real testing suite like it is done for other frameworks should be the way to go.

Documentation changes

Will probably need a new tab on backend frameworks examples.

Checklist for important updates

  • Changelog has been updated
  • coreDriverInterfaceSupported.json file has been updated (if needed)
    • Along with the associated array in lib/ts/version.ts
  • frontendDriverInterfaceSupported.json file has been updated (if needed)
  • Changes to the version if needed
    • In package.json
    • In package-lock.json
    • In lib/ts/version.ts
  • Had run npm run build-pretty
  • Had installed and ran the pre-commit hook
  • Issue this PR against the latest non released version branch.
    • To know which one it is, run find the latest released tag (git tag) in the format vX.Y.Z, and then find the latest branch (git branch --all) whose X.Y is greater than the latest released tag.
    • If no such branch exists, then create one from the latest released branch.
  • If have added a new web framework, update the add-ts-no-check.js file to include that
  • If added a new recipe / api interface, then make sure that the implementation of it uses NON arrow functions only (like someFunc: function () {..}).
  • If added a new recipe, then make sure to expose it inside the recipe folder present in the root of this repo. We also need to expose its types.

@ColinEspinas ColinEspinas changed the base branch from master to 12.1 January 16, 2023 23:51
const request = new H3Request(event);
const response = new H3Response(event);
try {
await supertokens.middleware(request, response);
Copy link
Member

Choose a reason for hiding this comment

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

h3 middlewares seem to be compatible with express' middleware. Therefore, I think you can just copy over the logic from the express framework (but of course, use the H3Request and H3Response wrapper instead of ExpressRequest and ExpressResponse.

The key point here is that await supertokens.middleware(request, response) returns a boolean. If it's false, it means that supertokens didn't handle the request and you need to call the next function.

Comment on lines +183 to +184
const error = createError({});
sendError(event, error);
Copy link
Member

Choose a reason for hiding this comment

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

I think you can copy over the code from the express errorHandler cause the errorHandler too is a middleware that needs to be added to your h3 app after all your routes.

import type { SessionEvent } from "../../../framework/h3/framework";
import { H3Request, H3Response } from "../../../framework/h3/framework";
import SuperTokens from "../../../supertokens";
import { eventHandler } from "h3";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { eventHandler } from "h3";
import type { eventHandler } from "h3";

Copy link
Member

Choose a reason for hiding this comment

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

We are only allowed to import types from frameworks cause our package.json has h3 only in dev dependencies.

Comment on lines +16 to +29
import {
eventHandler,
send,
setCookie,
createError,
EventHandler,
H3Event,
sendError,
readBody,
getQuery,
getCookie,
getHeader,
getMethod,
} from "h3";
Copy link
Member

Choose a reason for hiding this comment

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

Our SDK cannot depend on these functions cause we can't add dependency on h3 in our package.json (but we can in dev dependencies - which you already have). Is there a way in which these functions can be called directly on the H3Event object? For example event.readBody()

try {
await supertokens.middleware(request, response);
} catch (err) {
await supertokens.errorHandler(err, request, response);
Copy link
Member

Choose a reason for hiding this comment

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

In the discord logs you had sent earlier, it seemed that the code execution came in the error handler (which is expected), and that this function wrote status code 401 to the response object, but you still got back a 404. This could possibly be because:

  • Setting the status code on the response object and sending a reply doesn't actually send it to the client (maybe a bug in the H3Response implementation above)
  • h3 routing may require you to define paths for all routes that can be called from the frontend, else it returns a 404. This behaviour is similar to other frameworks like Hapi in which we push all the routes dynamically as seen here.

@rishabhpoddar rishabhpoddar changed the base branch from 12.1 to 13.1 February 22, 2023 15:36
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