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: tree shake unused API methods from CLI #6973

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Feb 8, 2024

Ref #6647

This switches the OpenAPI generator used to https://github.com/oazapfts/oazapfts. The official openapi-generator creates very verbose code. This code is much smaller as you can see by the lines added and deleted in this PR. The other issue with the official generator is that it generates code in such a way that bundlers cannot remove unused code. See OpenAPITools/openapi-generator#17800 for more details on that. With this PR, we only keep the code we actually use.

This will allow us to migrate the web API incrementally. E.g. if we leave the axios client for upload/download in the short-term, but replace the other usages, while that still leaves a lot of page weight it will overall be much less. We can also focus on migrating the most used pages first. This PR removes almost 160 kB from the CLI.

main
dist/index.js 764.81 kB

PR
dist/index.js 606.80 kB

In the web project, the API chunk is one of the largest produced, so being able to shrink it would be a big win.
.svelte-kit/output/client/_app/immutable/chunks/api.tiNJRPXV.js 134.81 kB

There are two drawbacks I ran into with this library:

@alextran1502 alextran1502 added the cli Tasks related to the Immich CLI label Feb 8, 2024
@etnoy
Copy link
Contributor

etnoy commented Feb 8, 2024

I'm fine with all these changes.

@jrasm91
Copy link
Contributor

jrasm91 commented Feb 8, 2024

The web uses probably 95%+ of the endpoints, so I'm not sure how much it would benefit from tree shaking.

The CLI only uses 3-4 endpoints, but it isn't a website that needs to be optimized for first page load.

This seems like potentially a big change that would require undoing recent changes and it is unclear to me exactly what the benefit would be.

Id probably prefer some more analysis on how many unused endpoints we could tree shake out before committing to such a large migration. It doesn't help that the web has no e2e tests that would catch any regression issues so the risk of breaking something seems much higher.

@benmccann
Copy link
Contributor Author

It will still be a very large benefit on the web because SvelteKit/Vite does code splitting and only loads the code required for a page. That means that any page should only need to load the endpoints that it actually uses. However, with the current API generator we end up loading basically the entire API for every page instead of just the few endpoints used on that particular page. I actually started down this whole project after noticing this issue in the web project. I started with the CLI first just because it was smaller, so easier for me to get started with.

Comment on lines +51 to +53
async addAssetsToAlbum(id: string, bulkIdsDto: BulkIdsDto) {
return await addAssetsToAlbum({ id, bulkIdsDto }, this.options);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove async and await from all of these methods. I prefer not having any of those unless they're actually needed.

import * as Oazapfts from "oazapfts/lib/runtime";
import * as QS from "oazapfts/lib/runtime/query";
export const defaults: Oazapfts.Defaults<Oazapfts.CustomHeaders> = {
headers: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

This object exists for stuff like setting up auth and what not. I'm pretty sure we can use this instead of keeping the ImmichApi class around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ImmichApi class is actually pretty helpful for testing as we can mock the functions on it. If the code directly imported the functions we'd need to figure out some solution for mocking them

I also was a little wary of using the defaults object because it's a global and I didn't know how well that would work if we ran multiple tests in parallel that ended up stomping on each other's options

Copy link
Contributor

Choose a reason for hiding this comment

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

while true, I thought the main reason was to help with code splitting. if the web has to use a class to enable easier testing than none of this will really be beneficial in that regard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I'd take a different approach in the web project. here it was fine to stick everything in a single class because we're just bundling to a single distributable

@jrasm91 jrasm91 merged commit aff71a1 into immich-app:main Feb 9, 2024
25 checks passed
@benmccann benmccann deleted the oazapfts branch February 9, 2024 21:16
@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.

4 participants