Skip to content

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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

achepukov
Copy link
Contributor

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
Copy link
Contributor Author

@jwagner wdyt?

@achepukov
Copy link
Contributor Author

@jwagner any other ideas how it could be improved?

@jwagner
Copy link
Owner

jwagner commented Sep 5, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@jwagner sounds like your approach solves extra issue 👍

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