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

PERF: switch from axios to fetch #6647

Closed
3 tasks done
benmccann opened this issue Jan 26, 2024 · 4 comments · Fixed by #7490
Closed
3 tasks done

PERF: switch from axios to fetch #6647

benmccann opened this issue Jan 26, 2024 · 4 comments · Fixed by #7490

Comments

@benmccann
Copy link
Contributor

The bug

The typescript-axios generator pulls in an extra dependency on axios. The typescript-fetch generator could avoid this by using the browser's built-in fetch method.

Note that fetch is not supported in Internet Explorer (https://caniuse.com/fetch). I didn't see in the docs which browsers are supported, but I'm guessing IE is not a concern since it's a fairly recent app

The OS that Immich Server is running on

any

Version of Immich Server

any

Version of Immich Mobile App

any

Platform with the issue

  • Server
  • Web
  • Mobile

Your docker-compose.yml content

n/a

Your .env content

n/a

Reproduction steps

https://github.com/immich-app/immich/blob/main/open-api/bin/generate-open-api.sh
https://github.com/immich-app/immich/blob/ca28e1e7a82b4467012e27934c2cddda39ea2c6c/.github/workflows/dispatch_sdk_update.yml#L23

Additional information

No response

@jrasm91
Copy link
Contributor

jrasm91 commented Jan 26, 2024

I'd be fine with this, just implies testing and validating the new client. Specifically, the file/attachment upload endpoints have always been tricky with the auto-generated clients.

@benmccann
Copy link
Contributor Author

benmccann commented Feb 1, 2024

I took a look at this and overall it's quite nice as it removes the need to do .data on every response.

The trickiest thing is figuring out how to re-implement onUploadProgress and onDownloadProgress from axios. Firefox/Safari don't have full support for the necessary fetch APIs (one only supports upload and the other only supports download):

So the only way to monitor progress of these large file transfers appears to be to drop down and use XHR directly for that API call rather than using the typescript-fetch API for that specific call.

@jrasm91
Copy link
Contributor

jrasm91 commented Feb 1, 2024

That might be a reasonable compromise. So far we have had to manually re-implement those endpoints to some extent anyways. Another option (maybe in the short term). Is to only use axios for those two requests and eventually substitute them out later.

@benmccann benmccann changed the title PERF: switch from typescript-axios to typescript-fetch OpenAPI Generator PERF: switch from axios to fetch Feb 11, 2024
@benmccann
Copy link
Contributor Author

benmccann commented Feb 11, 2024

I migrated the CLI from the typescript-axios to typescript-fetch, but which removed the axios dependency. However, I saw that unused API methods weren't being tree shaken out, so switched to oazapfts, which does support tree shaking and produces output that's only 1/10 the size of typescript-fetch even before tree shaking.

I filed oazapfts/oazapfts#571 as a feature request to ease the transition for APIs that use enums. We could still migrate without that, but it would require some more code changes. I also sent oazapfts/oazapfts#568 to reduce code size a bit, but that's not a blocker at all.

For the most part, the migration should be pretty straightforward. We just need to replace @api with @immich/sdk and update calls like api.getPeopleThumbnailUrl to use getPeopleThumbnailUrl included via import { getPeopleThumbnailUrl } from '@immich/sdk'. The one thing that will be harder is progress tracking: #6647 (comment)

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 a pull request may close this issue.

2 participants