Typescript: Incorrect usage of Union Typing (mostly options)

[CodeMan99]

Describe the bug in a sentence or two.

Many options type arguments are using union when the intention is an intersection.

Issue Type (Can be multiple)

• [ ] Build - Can’t install or import the SDK
• [ ] Babel - Babel errors or cross browser issues
• [ ] Performance - Performance issues
• [ ] Behaviour - Functions aren’t working as expected (Such as generate URL)
• [ ] Documentation - Inconsistency between the docs and behaviour
• [x] Incorrect Types - For typescript users who are having problems with our d.ts files
• [ ] Other (Specify)

Steps to reproduce

An example using the ConfigAndUrlOptions type.

import { v2 as cloudinary, ConfigAndUrlOptions } from 'cloudinary';

const options: ConfigAndUrlOptions = {
    version: "1",
    cloud_name: "something",
    // ConfigOptions type is `boolean`, here it is a string
    force_version: "true",
};

// look at typescript tooltip for the options object
cloudinary.url('puclic_id', options);

This example will not fail to compile. However, force_version is provided as a string.

Correct Usage

Instead of a union, the ConfigAndUrlOptions object should be an intersection.

import { v2 as cloudinary, ConfigOptions, UrlOptions } from 'cloudinary';

type ConfigAndUrlOptions = ConfigOptions & UrlOptions;

const options: ConfigAndUrlOptions = {
    version: "1",
    cloud_name: "something",
    // typescript compile error, wrong type provided
    force_version: "true",
};

cloudinary.url('puclic_id', options);

Wrap Up

There are a lot of unions being mis-used this way. I'm happy to help create a PR. However, doing so would take significant time for me.

[aleksandar-cloudinary]

Hi @CodeMan99 - thanks for reporting this! As I understand, this is not causing you any compilation errors currently? I will create a ticket internally for our SDK team so we can look into this. That said, due to a number of higher priority tickets this may not be addressed right away. If you do want to submit a PR then that would also be welcome but if not, our team will look into it.

[CodeMan99]

Correct. This is not a show stopper.

Just would be a very nice to have for better type support.

[aleksandar-cloudinary]

No worries @CodeMan99 - we will update this thread when there are updates on our side.

[msrumon]

I was about to create a new issue, then I found this thread and thought: well, mine is kinda relevant to this one, so let's do it here. According to the doc, the upload function accepts both file name (as string) and file stream (as Buffer). However, the types only say string at lines 1031 and 1033. I believe it should be:

function upload(file: string | buffer, options?: UploadApiOptions, callback?: UploadResponseCallback): Promise<UploadApiResponse>;

Think about that. I'm willing to send a PR if needed.

Update:

const image = new Buffer(...); // contains buffer of a real image
cloudinary.v2.uploader.upload(image.toString())

yields The argument 'path' must be a string or Uint8Array without null bytes. Received '����\x00\x10JFIF\x00\x01\x01\x00\x00\x01\x00\x01\x00\x00��\x02(ICC_PROFILE\x00\x01\x01\x00\x00\x02\x18\x00\x00\x00\x00\x040\x00... error.

[aleksandar-cloudinary]

Hi @CodeMan99 - An update on the original query - after speaking with our team they advised that because we're using [futureKey: string]: any; which allows us in a way to future proof the types in case of changes going forward. This is also the reason why you're not seeing any type errors currently. That said, if this changes in future we will be sure to update this thread.

Hi @msrumon - Thanks for reporting this. Definitely, feel free to submit a PR and we can ask our team to review it. In regards to your update - could you try to convert it to a typedarray: var byte_arr = new Uint8Array(Buffer.from(...)); and then pass that as the "file" parameter when calling upload()?

You can also use the upload_stream method like so:

let cloudinary = require("cloudinary");
let streamifier = require('streamifier');

let uploadFromBuffer = (req) => {
   return new Promise((resolve, reject) => {
     let cld_upload_stream = cloudinary.v2.uploader.upload_stream(
      {
        // optional upload() method parameters
      },
       (error: any, result: any) => {
         if (result) {
           resolve(result);
         } else {
           reject(error);
         }
       }
     );
     streamifier.createReadStream(req.file.buffer).pipe(cld_upload_stream);
   });
};

async function uploadFile(req) {
  let result = await uploadFromBuffer(req);
  console.log(result);
}

uploadFile(req);

[msrumon]

Yes, I ended up using upload_stream method.

import { parse } from 'path'
import { Readable } from 'stream'

import { UploadApiOptions, v2 as cloudinary } from 'cloudinary'
// other imports //

export class CloudinaryDriver implements CloudinaryDriverContract {
  // other implementations //

  public async put(location: string, contents: string | Buffer, options?: WriteOptions) {
    try {
      const path = parse(location)

      const upOptions: UploadApiOptions = {
        public_id: options && 'name' in options ? parse(options.name).name : path.name,
        folder: path.dir || undefined,
        unique_filename: false,
      }

      if (Buffer.isBuffer(contents)) {
        await new Promise((resolve, reject) => {
          const stream = cloudinary.uploader.upload_stream(upOptions, (err, res) => {
            if (err) {
              reject(err)
            } else {
              resolve(res)
            }
          })

          Readable.from(contents).pipe(stream)
        })
      } else {
        await cloudinary.uploader.upload(contents, upOptions)
      }
    } catch (error) {
      throw new CannotWriteFileException(error)
    }
  }
}

However, I'll play around with the upload method and see if I can somehow utilize it.

[aleksandar-cloudinary]

Sounds good @msrumon - let us know how it goes. If you convert the Buffer to a Typedarray then upload() should be able to accept that. Otherwise, if you don't have the file stored locally on the filesystem and you're only getting a stream then upload_stream() is the way to go.