roubachof / NukeProxy

MIT License
16 stars 12 forks source link

Allow nullable urls #8

Closed J-Swift closed 1 year ago

roubachof commented 1 year ago

I'm not sure why we would want to allow null urls ...

Cheesebaron commented 1 year ago

So you want to allow to load a null and then you expect Nuke to use the specified error image?

That will not work as Nuke will throw since it doesn't allow nullable URLs.

J-Swift commented 1 year ago

I'm not sure why we would want to allow null urls ...

Its a much more ergonomic API for the user. e.g. in collection views with user provided images you don't have to worry about if the user has an image or not, you just set appropriate placeholder/error images and configure your cells. This is the same pattern that Glide follows. This also lets you appropriately cancel image requests that were set on previous cells without taking null into account, since the new request will take over.

That will not work as Nuke will throw since it doesn't allow nullable URLs.

This has not been the case in my testing. I was using a local patch until the net6 branch got merged with these changes.

J-Swift commented 1 year ago

You can see a motivating example in the changelog which I guess introduced optional urls: https://github.com/kean/Nuke/blob/master/Documentation/Migrations/Nuke%2010%20Migration%20Guide.md#optional-request

// Before (Nuke 9)
if let url = URL(string: string) {
    Nuke.loadImage(with: url, into: imageView)
} else {
    imageView.image = failureImagePlaceholder
}

// After (Nuke 10)
Nuke.loadImage(with: URL(string: string), into: imageView)
Cheesebaron commented 1 year ago

Right. I forgot I had bumped the Nuke dependency.

We might want to revisit the use of Nuke in general in the proxy. Also ran into some of the callbacks can return null, but the return types are not marked nullable and of course started to encounter null ref in production :')

I think we will wait with merging this one and plan a little overhaul.

Cheesebaron commented 1 year ago

Fixed in 51f91a9dcd53dd02c718a691657b3db663d2d15f