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

chore: remove axios dependency from CLI #6888

Merged
merged 1 commit into from Feb 5, 2024

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Feb 3, 2024

Ref #6647

This generates a second version of the API in the open-api project that uses the fetch API that is built into Node and the browser rather than axios. This migrates the CLI to the new API and leaves the web project on the old API. We can later migrate the web project as well and then delete the old axios API. It was too big of a change to figure out how to migrate both in one go, so this more incremental approach was easier.

The fetch API is nicer because it removes a bunch of unnecessary .data accesses and it removes a dependency resulting in faster project installs and less security risk due to a smaller attack area. It will make a bigger difference in the web project where removing a dependency will speed page load times

main: dist/index.js 1,817.04 kB
This PR: dist/index.js 1,088.98 kB

@bo0tzz bo0tzz added the cli Tasks related to the Immich CLI label Feb 3, 2024
Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

I haven't tested it yet, but this looks like a good approach. Have you been able to run/test the cli locally yet? I know fetch and axios handle errors differently. Have you been able to validate how errors handling works with the new client?

@benmccann
Copy link
Contributor Author

I believe Axios will automatically throw an exception where as fetch does not and you have to manually check the status code. However, the generated OpenAPI client checks the status code and throws and exception if the response wasn't okay, so in practice they end up being the same since we use them via the OpenAPI client. You can see in this PR there was one place where the underlying API was being used directly and there I had to check the status code

@etnoy
Copy link
Contributor

etnoy commented Feb 5, 2024

I tried it out and it seems to work well. Verified server-info and uploading assets.

@etnoy
Copy link
Contributor

etnoy commented Feb 5, 2024

This is great and helped me fix my outstanding issues with #6858. I suggest we merge this now and then I can merge my PR and all will be happy.

Thanks Ben!

@etnoy etnoy mentioned this pull request Feb 5, 2024
@etnoy etnoy merged commit 6ed33da into immich-app:main Feb 5, 2024
23 checks passed
@benmccann benmccann deleted the rm-axios-cli branch February 5, 2024 19:47
@jrasm91 jrasm91 added maintenance and removed cli Tasks related to the Immich CLI labels Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants