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

#353 - Add OasstApiClient interaction with task #421

Merged
merged 3 commits into from
Jan 6, 2023

Conversation

Klotske
Copy link
Contributor

@Klotske Klotske commented Jan 5, 2023

#353
My attempt at adding a taskInteraction method with its use in api/update_task.ts and contract test.

This is my first time working with cypress, but I hope I understood the idea.

@Klotske Klotske changed the title Add oasst api interaction #353 - Add oasst api interaction Jan 5, 2023
@Klotske Klotske changed the title #353 - Add oasst api interaction #353 - Add OasstApiClient interaction with task Jan 5, 2023
@@ -18,6 +19,8 @@ const handler = async (req, res) => {
return;
}

const oasstApiClient = new OasstApiClient(process.env.FASTAPI_URL, process.env.FASTAPI_KEY);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of creating a new client object every render, can we create a single client object in oasst_api_client.ts and export that? This is similar to how we do things with the prisma client.

Copy link
Collaborator

@jack-michaud jack-michaud Jan 6, 2023

Choose a reason for hiding this comment

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

We could do this, but if we wanted to add the 'user' to the constructor (as mentioned in #353 (comment)) it would need to be reworked.

Is there some sort of request context that we could add the api client to? Maybe using middleware?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need to provide user in the constructor of the client. Keeping it as an argument to the method calls seems fine. The cleanup we need is just for the usage of user to check what type of user is it and send a different user ID if they're authenticated via discord.

NextJS Middleware seems too intrusive since we don't need this to happen for every single API route.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be fixed by 3c330dc
Added prisma-like global variable init & changed exports so both oasstApiClient and OasstApiClient class could be used.

@fozziethebeat fozziethebeat merged commit 8a5b7ed into LAION-AI:main Jan 6, 2023
@Klotske Klotske deleted the add-oasst_api-interaction branch January 6, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants