Add onLoad and onError callbacks

[bildja]

1. I need to show and to do something once image has been loaded and therefore I need a callback for it.
2. I need to send a message to error logger, when image couldn't load.

[codecov]

Codecov Report

Merging #192 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #192      +/-   ##
==========================================
+ Coverage   98.14%   98.21%   +0.06%     
==========================================
  Files           1        1              
  Lines          54       56       +2     
==========================================
+ Hits           53       55       +2     
  Misses          1        1

Impacted Files	Coverage Δ
src/index.js	98.21% <100%> (+0.06%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef128e4...d3ee6d0. Read the comment docs.

[mbrevda]

I'm wondering if there is a more general way of approaching this. Could you call the logger from the fallback element?

[bildja]

@mbrevda yes, I think I could. Though it still leaves the need of onLoad (and this is why I did this PR at the first place), and image API seems to be more consistent with both onLoad and onError. Besides, I am not sure if sending a log is more of responsibility of the fallback element rather then the whatever component renders the image

[mbrevda]

I am not sure if sending a log is more of responsibility of the fallback element rather then the whatever component renders the image

Considering SRP, would it seem true that this lib's responsability is simply loading images/fallbacks? In which case, we can use composition to layer additional responsibilities.

A yet unreleased prop would act as an image wrapper, allowing post processing scenarios such as logging and animations.

To use, create a log element which receivse these props. When it loads, if children was passed , render them (it's a pre/unloader) and log an error (if the unloader was passed). If the above props were passed, mainly src, log that the image was loaded and render it (return <img {...this.props} />).

Would that work for your use case?

[bildja]

@mbrevda

Its responsibility will still be simply loading images/fallbacks. The component itself will not start doing logs/any other unrelated start to it. It will just give a consumer of it an ability to do it.
And it looks like with the container prop it would be possible to do what I need. Still, I'll suggest you to compare:

class MyComponent extends React.Component {
  render() {
    <div>
       <Img src={this.props.imageSrc} onLoad={() => this.setState({showAnotherThing: true})} onError={sendErrorLog} />
        {this.state.showAnotherThing ? <AnotherThing/> : null}
    </div>
  }
}

vs

class MyComponent extends React.Component {
  render() {
    <div>
       <Img src={this.props.imageSrc} fallbackComponent={WrappedFallbackComponent} container={children => children; /* and I need to find a way to setState for showAnotherThing here for example */}>
        {this.state.showAnotherThing ? <AnotherThing/> : null}
    </div>
  }
}

WrappedFallbackComponent = withErrorLogger(FallbackComponent);

[mbrevda]

Setup would actually be similar to this:

// image wrapper for images that have loaded succesfully
class ImageOkContainer extends Component {
  componentDidRender(){
     // update logging server that we have show the image
    if (this.props.src) console.log('Image loaded successfully!')
  }

  render() {
    return this.props.children
      ? this.props.children()
      : <img {...this.props} />
  }
}

// unloader, used as a fallback if all images fail to load
class Unloader extends Component {
  componentDidRender(){
    // log failed to load error
    console.error('Image failed to load!')
  }

  render() {
    return <span>Image failed to load!</span>
  }
}

// demo component
<Img
  src={this.props.imageSrc}
  loader={<span>Loading...</span>}
  unloader={Unloader}
  container={ImageContainer}
/>

[bildja]

@mbrevda this is pretty much what I showed. So first, comparing the two approaches, the one with callbacks is very concise and very react-way and second is verbose and also kind of react way, but in a bit different way and I'd stick with the callbacks.
And, second, example with ImageOkContainer does not actually solve the case where I need to setState on the component that renders <Img, only thing, so I would need to pass this callback to the container property anyway and it'd be really awkward

[mbrevda]

To pass the loaded state back to the parent, you could do:

// image wrapper for images that have loaded succesfully
class ImageOkContainer extends Component {
  componentDidRender(){
     // update logging server that we have show the image
    if (this.props.src) {
      console.log('Image loaded successfully!')
      this.props.parentNotifier(true)
    }
  }

  render() {
    return this.props.children
      ? this.props.children()
      : <img {...this.props} />
  }
}

class MyComponenet extends Component {
  constructor(){
    this.setImageLoadState = this.setImageLoadState.bind(this)
  }

  setImageLoadState(imageLoaded) {
    this.setState({imageLoaded})
  }

  render() {
    <Img
      src={this.props.imageSrc}
      loader={<span>Loading...</span>}
      unloader={Unloader}
      container={props => <ImageContainer {...props} parentNotifier={this.setImageLoadState} />}
    />
  }
} 

[mbrevda]

also, I dont understand what showAnotherThing is in your example. Isn't that what the unloader is for?

I also wonder if what your trying to do can be accomplished today: you can use the unloader to log errors and display an alternate component, and ab onLoad prop passed to Img will be passed to the underlying <img> when it loads, allowing you to log successful loads. While those props aren't "synchronous", I'm not sure adding error reporting is something that should be included by default when it could be so elegantly composed.

[bildja]

@mbrevda yes, that's what I meant by "I would need to pass this callback to the container property anyway". So, you see the difference between the example you give me and an example with callbacks?
Regarding your question "what showAnotherThing is in your example":
For example, currently I have a feature where I render two things:

1. Rectangles around faces on top of the image. While I can agree it logically can belong inside the <Img /> component as a child, I have reasons to put it in a different level.
2. A toggle element which turns on/off the faces on top of images and it belongs very different level on the component tree, so I need to only show it by a state flag, when the image is loaded.

If I used just <img /> element what I would do is exactly passing onload={} and onerrror={} properties and setting the state value accordingly

[mbrevda]

So, you see the difference between the example you give me and an example with callbacks?

Perhaps I'm not following, but I'm not convinced that anything needs to be added here. Regardless of the method, you'll need to use callbacks to hoist the state up to the parent.

[dum3ng]

I would also like to the onLoad and onError way.
It does not add more functionality to this component but just provide a interface for users to do other things.
The way with a container is not a clear way, at least to me.

[mbrevda]

onLoad is currently supported. I've left this PR open to give it more though.

[mbrevda]

It is now possible to inject a custom image loader with any desired behavior. You can pass it as a prop to the useImage hook or to the component, see more here.

[mbrevda]

It is now possible to inject a custom image loader with any desired behavior. You can pass it as a prop to the useImage hook or to the component, see more here.