catdad-experiments / heic-convert

🤳 convert heic/heif images to jpeg and png
245 stars 24 forks source link

Export isHEIC method #12

Closed hisham closed 3 years ago

hisham commented 3 years ago

Before deciding to convert HEIC we need to first determine if the image is HEIC.

I could check mime, etc..but it seems your code already has an isHEIC method but it's not exported.

Currently you just throw an exception if image is not HEIC. And that exception is of type TypeError and has a string.

It would be great if you either expose isHEIC method or throw a more specific exception. Currently I'm catching the exception and parsing the message to determine if image is not HEIC, which is a brittle approach to determine if image is HEIC.

catdad commented 3 years ago

Maybe I am missing something. What is not specific or is brittle about TypeError: input buffer is not a HEIC image? This is part of the API surface of the module, complete with tests, and subject to semver versioning.

hisham commented 3 years ago

Here's my method that converts HEIC to jpeg using your lib.

As you see I'm parsing the error message to determine if the error is of the type not HEIC. It feels like I'm depending on some private implementation of yours that can change anytime.

Perhaps I should just use if (error instanceof TypeError) and assume anytime you throw TypeError it's because image is not HEIC?

  public async convertHEICToJpeg(buffer: Buffer): Promise<Buffer> {
    let result = buffer;
    try {
      result = await convert({ buffer, format: 'JPEG' });
    } catch (error) {
      if (!(error?.message as string).endsWith('not a HEIC image')) {
        throw error;
      }
    }
    return result;
  }
catdad commented 3 years ago

This error is an official part of the module's API, and there is in fact a test for it: https://github.com/catdad-experiments/heic-convert/blob/02907bee8ae01904172fd01d54eae7e0de36e201/test/index.test.js#L135-L146

Just like any other part of the API, it will not change in a minor release (of course, any part of the API may change in a major release), so it is safe to use.

I am not sure that adding extra methods in the API is right in this case. There are other modules for determining a file mime type based on its content, such as file-type.