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 form-data dependency #6876

Merged
merged 1 commit into from
Feb 3, 2024
Merged

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Feb 3, 2024

This uses the standard built-in class for FormData vs an external package. It will also let us remove axios as a dependency in a follow-up (#6647).

@benmccann benmccann force-pushed the rm-form-data branch 2 times, most recently from 0de5e71 to 8330069 Compare February 3, 2024 05:32
@benmccann benmccann mentioned this pull request Feb 3, 2024
@mertalev mertalev added the cli Tasks related to the Immich CLI label Feb 3, 2024
@mertalev mertalev merged commit a4cfb51 into immich-app:main Feb 3, 2024
23 of 24 checks passed
@jrasm91
Copy link
Contributor

jrasm91 commented Feb 3, 2024

Does this still stream the file from disk? Also, ever since the CLI was converted to esm it no longer works. TSC doesn't modify the import paths and esm requires extensions. Can we use vite to bundle the CLI to fix this?

@benmccann
Copy link
Contributor Author

Good call outs. Let me check into these.

How about if we add the file extensions rather than bundling? I can update the tsconfig to fail the build if the file extensions are missing

@benmccann
Copy link
Contributor Author

Although maybe bundling is better as it will result in a smaller distributable. I'm not quite sure how the CLI is distributed currently

@jrasm91
Copy link
Contributor

jrasm91 commented Feb 3, 2024

Good call outs. Let me check into these.

How about if we add the file extensions rather than bundling? I can update the tsconfig to fail the build if the file extensions are missing

I already looked into this, but the generated typescript-sdk is typescript files, which don't have extensions in the imports, so I'm not sure how to address the issue in that code. The rest of it was easy enough to change.

@benmccann
Copy link
Contributor Author

Does this still stream the file from disk?

It seems we are very lucky to have to support only Node 20, which makes life here vastly easier. This PR uses openAsBlob which should allow for streaming the data: nodejs/undici#2202 (comment). We do wrap the result of that in new File, so I don't know if we'd need to remove that or not. If it's not streaming, it seems like it'd probably be a small change to make it do so. I could probably use some help in testing whether it does stream or not

@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.

3 participants