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

website: Combine fetcher and poster files, update all references to use axios for both GET and POST #663

Merged
merged 1 commit into from
Jan 13, 2023

Conversation

rjmacarthy
Copy link
Contributor

@rjmacarthy rjmacarthy commented Jan 12, 2023

Hi there,

Firstly, thank you for the amazing work you are doing it's truly inspirational and I'm happy to help if I can. After browsing the front-end code I propose a change. I've made some updates to the application in this PR and I wanted to share them with you.

I've combined the fetcher and poster files into one file, api.ts, and standardised the use of axios for both GET and POST requests. This should simplify the code and make it easier to maintain going forward. I've also updated all references to these methods throughout the codebase. I noticed that data is run through JSON.stringify(arg) I've removed the need for it. By creating an axios api instance we can setup an interceptor which controls the response and reject in one place.

Please let me know if you have any questions or concerns, and I'll be happy to address them.

Thank you

@github-actions
Copy link

pre-commit failed.
Please run pre-commit run --all-files locally and commit the changes.
Find more information in the repository's CONTRIBUTING.md

@rjmacarthy rjmacarthy marked this pull request as draft January 12, 2023 23:08
Copy link
Collaborator

@fozziethebeat fozziethebeat left a comment

Choose a reason for hiding this comment

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

Thank you! I like this cleanup.

You'll need to merge against main as I think you fixed a bug someone else just fixed.

@fozziethebeat
Copy link
Collaborator

Also, from the build failure it looks like there's still an import site for src/lib/fetcher. That'll need to be fixed too.

@github-actions
Copy link

pre-commit failed.
Please run pre-commit run --all-files locally and commit the changes.
Find more information in the repository's CONTRIBUTING.md

@rjmacarthy rjmacarthy marked this pull request as ready for review January 13, 2023 09:44
@rjmacarthy
Copy link
Contributor Author

Also, from the build failure it looks like there's still an import site for src/lib/fetcher. That'll need to be fixed too.

Thank you for your review, I have addressed this and updated my pull request.

Copy link
Collaborator

@fozziethebeat fozziethebeat left a comment

Choose a reason for hiding this comment

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

The cypress contract tests break due to something changed in oassist_api_client.ts Can you investigate?

@fozziethebeat
Copy link
Collaborator

If possible, maybe revert the changes to the oassist_api_client.ts. That's used only on the API routes and not on the client side so it's actually not using axios, it's using node-fetch. That library is working well enough and doesn't need refactoring. But our useSWR callsites definitely need cleaning up.

@github-actions
Copy link

pre-commit failed.
Please run pre-commit run --all-files locally and commit the changes.
Find more information in the repository's CONTRIBUTING.md

@rjmacarthy rjmacarthy marked this pull request as draft January 13, 2023 17:01
@github-actions
Copy link

pre-commit failed.
Please run pre-commit run --all-files locally and commit the changes.
Find more information in the repository's CONTRIBUTING.md

Fix set_label id missing in payload use frontend_message_id

pre-commit

Refactor api fetcher/poster to axios create

lint

Remove string literal for path

Revert oasst_api_client.ts

Fix warning httpStatusCode OasstError optional parameter

Refactor remove api base url for local api

Lint add blank line
@rjmacarthy
Copy link
Contributor Author

rjmacarthy commented Jan 13, 2023

If possible, maybe revert the changes to the oassist_api_client.ts. That's used only on the API routes and not on the client side so it's actually not using axios, it's using node-fetch. That library is working well enough and doesn't need refactoring. But our useSWR callsites definitely need cleaning up.

Thank you. I have reverted the change to this file and updated the code to remove the dependency of environment variables for non proxied api calls. A more structured way in the future might be generic hook to call the api which takes an options parameter for any special clauses.

@rjmacarthy rjmacarthy marked this pull request as ready for review January 13, 2023 17:09
Copy link
Collaborator

@fozziethebeat fozziethebeat left a comment

Choose a reason for hiding this comment

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

Thank you for the changes! everything passes!

@fozziethebeat fozziethebeat merged commit db5df84 into LAION-AI:main Jan 13, 2023
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