immich icon indicating copy to clipboard operation
immich copied to clipboard

PERF: switch from `axios` to `fetch`

Open benmccann opened this issue 1 year ago • 4 comments

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

  • [X] Server
  • [X] Web
  • [X] 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

benmccann avatar Jan 26 '24 00:01 benmccann

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.

jrasm91 avatar Jan 26 '24 01:01 jrasm91

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):

  • https://github.com/axios/axios/issues/1219#issuecomment-1281827059
  • https://developer.mozilla.org/en-US/docs/Web/API/Request/body
  • https://developer.mozilla.org/en-US/docs/Web/API/Response/body

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.

benmccann avatar Feb 01 '24 16:02 benmccann

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.

jrasm91 avatar Feb 01 '24 21:02 jrasm91

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 https://github.com/oazapfts/oazapfts/issues/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 https://github.com/oazapfts/oazapfts/pull/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: https://github.com/immich-app/immich/issues/6647#issuecomment-1921738727

benmccann avatar Feb 11 '24 17:02 benmccann