Skip to content

Add image upload support (/image API) - #46

Merged
tanysheng merged 4 commits into
masterfrom
implement-image-api
Sep 9, 2026
Merged

Add image upload support (/image API)#46
tanysheng merged 4 commits into
masterfrom
implement-image-api

Conversation

@tanysheng

Copy link
Copy Markdown
Contributor

Comment thread src/multipart.ts

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

To maintain supports for Node 7.x and newer, otherwise we could consider Node 18 which fetch is available natively

@tanysheng tanysheng changed the title Add image upload support (/image API) Add image upload support (/image API) Aug 24, 2026
@strzibny

Copy link
Copy Markdown
Contributor

Timeout and network errors are converted into generic ImageApiError... RequestTimeoutError, DNS errors, TLS failures, and connection reset shouldn't be just ImageApiError. We should fix that.

I also noticed that JSON response with error key is returned with HTTP 200 and resolves as a successful response, but the client should understand there is an error. But that's the whole library problem.

@tanysheng

Copy link
Copy Markdown
Contributor Author

Timeout and network errors are converted into generic ImageApiError... RequestTimeoutError, DNS errors, TLS failures, and connection reset shouldn't be just ImageApiError. We should fix that.

I also noticed that JSON response with error key is returned with HTTP 200 and resolves as a successful response, but the client should understand there is an error. But that's the whole library problem.

Good catch. I removed ImageApiError, so the error is now consistent with the search method. It still has some flaws (e.g. raw JSON), and I plan to fix them together in another PR, including cases where it's successful when HTTP 200 error is returned.

@strzibny

strzibny commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

It's fine to address in a follow-up. Can you rebase and make the test green?

@tanysheng

Copy link
Copy Markdown
Contributor Author

It's fine to address in a follow-up. Can you rebase and make the test green?

@strzibny Done

@strzibny strzibny left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM with a follow-up.

@tanysheng
tanysheng merged commit 9c08eb1 into master Sep 9, 2026
30 checks passed
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 this pull request may close these issues.

2 participants