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

Migrate to only use the upload endpoint, Add video parameter support #217

Open
Infinitay opened this issue Sep 10, 2023 · 3 comments
Open
Labels

Comments

@Infinitay
Copy link

Infinitay commented Sep 10, 2023

Is your feature request related to a problem? Please describe.
Unable to upload videos larger than 10MB

Describe the solution you'd like

  • When uploading an image or video, use the /image endpoint
    • Maybe not. Just noticed /image doesn't return useful info when trying to upload a video - the >10 MB video
  • Allow the user to specify either an image or video parameter as the Payload

Currently, users can't upload videos over 10 MB because regardless of the endpoint (/image or /upload) the image payload parameter is limited to 10 MB while the new video parameter has a limit of 200MB (https://apidocs.imgur.com/#c85c9dfc-7487-4de2-9ecd-66f727cf3139). By allowing users to specify whether they want to upload an image or video appropriately, not only would it be proper as opposed to sending a video as an image payload, but it would also allow for larger video files to be uploaded.

Additional context
Another thing, I was looking at the code and I wanted to let you know that axios can handle the form data on its own now by specifying the object as a payload. In our case, just make sure to maintain the form content header. For example,

const postData = {
	video: fs.createReadStream("./video.mp4") as any,
	title: "Test",
	description: "Test description",
	name: "video.mp4",
	type: "file",
};
const response = await axios.post("https://api.imgur.com/3/image", postData, {
	headers: { Authorization: `Bearer ${client.credentials.accessToken}`, "content-type": "multipart/form-data;" },
});

I was going to try and make the changes myself, but I didn't want to potentially break anything since supposedly the tests aren't working. The main confusion was with

  for (const [key, value] of Object.entries(payload)) {
    const supportedUploadObjectTypes = ['base64', 'stream'];
    if (supportedUploadObjectTypes.indexOf(key) !== -1) {
      if (supportedUploadObjectTypes.indexOf(payload.type as string) !== -1) {
        form.append(key, payload);
      }
    } else if (value) {
      form.append(key, value);
    }
  }

Specifically the checks to append the payload. I don't see how form.append(key, payload); is ever reached because isn't if (supportedUploadObjectTypes.indexOf(key) !== -1) never met? Checking the Payload type definition, the key could never be base64 or stream unless I am missing something else.

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@KenEucker
Copy link
Owner

KenEucker commented Oct 31, 2023

I agree that this would be a good addition to this library. I encourage you to create a pull request if you can.

Regarding that bit of code, I have been through that several times and I know it is confusing but I think it is correct. I am willing to collaborate with you on this and see if that code can be changed.

@Infinitay, The tests should be working and I just verified that myself. What about the tests are not working for you? (are you using Node 18+?) Nevermind, I can see now that all of the tests for the upload method are commented out. That's a problem.

@KenEucker
Copy link
Owner

Dissecting the code in question, from the createForm util function, I see that what this code is doing is looping through the payload and appending the values to send to the Imgur API to a form.

Here is the definition of the payload object:

export interface Payload {
  image?: string | Buffer | ReadableStream;
  type?: 'stream' | 'url' | 'base64';
  name?: string;
  title?: string;
  description?: string;
  album?: string;
  disable_audio?: '1' | '0';
}

So, the for loop for (const [key, value] of Object.entries(payload)) is looping through this payload object and appending the key, value pairs to the form.

The only key that can meet the criteria of if (supportedUploadObjectTypes.indexOf(key) !== -1) is the type key, in the payload, which you can see above is defined as type?: 'stream' | 'url' | 'base64'. This code, then, supports the upload types of base64 and stream, which you can see omits the url type. (which is not an upload)

To add the file type upload would not be difficult. My guess is that we just need to add that to the type definition and the supportedUploadObjectTypes array. I will add that and see what happens, but tests would need to be added for this code to be integrated into this library.

Finally, I like the idea of removing a dependency and simplifying the code if it is at all possible. However, the formData we create in this library is both used as the data in the request as well as in setting headers for the request, as seen here:

headers: form.getHeaders(),

That code is used in several other methods, and I am not sure that we can change it at this time.

@KenEucker
Copy link
Owner

@Infinitay, I started this work in a branch here:
https://github.com/KenEucker/imgur/tree/add-video-support

I needs more work to be functional and then some documentation added, at the least.

@polar-sh polar-sh bot added the Fund label Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants