jwagner / smartcrop-sharp

Node module for using smartcrop via sharp/libvips
MIT License
115 stars 14 forks source link

sharp peer dependency could be any in range 0 =< 1 #32

Open achepukov opened 1 year ago

achepukov commented 1 year ago

Sounds like sharp peer dependency needs manual resolve every time it's updated...

I googled around and found that using asterics is a bad practice

So, I think we are relatively safe with any version < 1, as 1 will be breaking change (fair to guess).

Also, this update does not enforce users who already work with old sharp version to update it, they are safe to keep using the old version.

Here is a nice version calculator to check if anything needed

achepukov commented 1 year ago

@jwagner wdyt?

achepukov commented 1 year ago

@jwagner any other ideas how it could be improved?

jwagner commented 1 year ago

Just had another idea on how this could be resolved. I could just change the interface so sharp is passed in as parameter (either directly to crop or to some factory). Using dependency injection that way I could just define the interface I expect of sharp in the type definition, and the direct dependency on sharp would be eliminated. If there were any incompatibilities in the future because of an API change on the sharp side, hopefully the type definitions would catch it - at least for typescript users.

The example from the README could the look like this:

// finds the best crop of src and writes the cropped and resized image to dest.
function applySmartCrop(src, dest, width, height) {
  return smartcropSharp(sharp).crop(src, { width: width, height: height })
    .then(function(result) {
      const crop = result.topCrop;
      return sharp(src)
        .extract({ width: crop.width, height: crop.height, left: crop.x, top: crop.y })
        .resize(width, height)
        .toFile(dest);
    })
}

Minimally less beginner friendly, but arguably still more so than fighting with broken peer dependencies. I think the main drawback would be that it's a breaking API change.

What do you think?

achepukov commented 1 year ago

I can live with that. I don't see this solution much optimistic, since it makes codebase a bit more complicated (import sharp, then inject it). Imo, peer dependency is cleaner. Would be great to check the review of somebody else :)

achepukov commented 1 year ago

@jwagner sounds like your approach solves extra issue 👍