Minor improvements suggestions

[bartlomiejzuber]

Hi @Jameskmonger , I've look at code and got few suggestions.
Please take a look at them. Don't know if contributes are welcome here so I would rather ask than making PR with those changes.

If you don't like them just close the issue.

1. Why componentDidUpdate instead of shouldComponentUpdate ?

    public async componentDidUpdate(prevProps: ImageProps) {
        if (this.props.src === prevProps.src) {
            return;
        }

        await this.renderImage();
    }

2. Wouldn't it be better to create ref inside constructor ? React.createRef() Right now each render creates new arrow function with ref, it's not a ideal for performance.
   The thing you used here is callback ref it's designed especially to pass it to inner components but here is vanilla DOM component not custom JSX component.

    public render() {
        return <a ref={el => this.container = el} href={this.props.src} className="loaded-image-container" />;
    }

3. If you really want to get best performance while loop on lastChild is faster. ;)

        while (this.container.firstChild) {
            this.container.removeChild(this.container.firstChild);
        }

[bartlomiejzuber]

Also by importing

import { Image } from "load-image-react";

I see that it includes more than 20KB. It's more than React lib itself.

It's really needed to include all https://github.com/blueimp/JavaScript-Load-Image inside that package ?
Maybe dynamic import would be better or limit it to only few things??

[Jameskmonger]

@bartlomiejzuber Thanks for this!

I agree with points 1 and 3. For point 2, it needs to call the renderImage function on props change (to update the container element), so componentDidUpdate is appropriate as it's not controlling whether or not to re-render the whole component

Also unfortunately I don't think we'll be able to reduce the import size because there's only a single export from blueimp-load-image.

[bartlomiejzuber]

For point 2, it needs to call the renderImage function on props change (to update the container element), so componentDidUpdate is appropriate as it's not controlling whether or not to re-render the whole component

Don't get it. You just do same thing on shouldComponentUpdate it got two parameters first one is:
nextProps, second one is: nextState.

So it would look like this:

public shouldComponentUpdate(prevProps: ImageProps) {
        return this.props.src === prevProps.src
}

public getSnapshotBeforeUpdate() {
    this.renderImage();
}

or

public shouldComponentUpdate(prevProps: ImageProps) {
        return this.props.src === prevProps.src
}

public componentDidUpdate() {
    this.renderImage();
}

Also do we need await for this.renderImage(); as we don't use any return from it?

Also unfortunately I don't think we'll be able to reduce the import size because there's only a single export from blueimp-load-image.

How about that? 🎉

vs

Thanks James that you are open for discussion.
I appreciate every response as I could learn from it. 👍

[Jameskmonger]

@bartlomiejzuber I agree that your example there with shouldComponentUpdate and componentDidUpdate is nice and clear

I don't think we can do those manual file imports because then we will lose the scripts for EXIF parsing etc