fix: validate image asynchronously by HTMLImageElement instead of XHR

[sebastianvera]

Use an HTMLImageElement instance to validate image URLs instead of
checking the response of a HEAD request. The problem with that approach
is that if the servers don't allow HEAD requests, no images will be
loaded even though they are valid URLs. This solution also performs this
validation process asynchronously so no more blocking the main thread by
performing XHR requests.

Fixes #145

[pixelbandito]

A couple of minor comments, but I think this looks good.

(I'd like to see one additional test.)

[pixelbandito]

I would have probably put the Image in a ref and used removeEventListener and a named function for the event handler.

But I can't think of a reason this wouldn't work, unless the anonymous function's closure does something weird with the state setter setLoadedImageUrl. I'll approve it, as long as you'll keep an eye out for weird side effects.

Maybe add a unit test for the case where the imageUrl changes after mounting the component?

[sebastianvera]

Didn't quite get the ref solution but I'm taking care of weird state updates (caused by race conditions) by removing the onload and onerror callbacks in the cleanup function every time imageUrl changes. I can add a new test case, but I'll need to add two new props onLoad and onError, so it can be easily tested 🙂

[sebastianvera]

I'm not sure how to test it, I'll need to add a delay so I can change the imageUrl prop before onload gets called, any idea on how to test that?

[pixelbandito]

I think you're ok as it is. I can see that being a pain to test.
You were at least able to validate in storybook, right?

[sebastianvera]

I did validate it in Storybook. Anyway, I figured out a way of testing it, let me know if you like it, otherwise, we revert it.

[sebastianvera]

cc: @pixelbandito

[javierjah]

Great work. No blocking the main thread anymore 🚀

[pixelbandito]

lgtm. Thanks for the extra test!

[sebastianvera]

@pixelbandito How should we proceed? I cannot merge this

[pixelbandito]

Hey @sebastianvera, there are a couple of small extra things you could do.

1. Run a local test importing your version of RoverUI's Avatar into the Cision React app's front-end. This isn't strictly necessary since your changes will be in a new version, but it's helpful.
2. Bump the version and tag in your branch.

Then I'll take care of merging to master and publishing to npm.

I just opened a branch with some updated instructions on the README that should be helpful.
Yalc testing
Version bumps

[sebastianvera]

@pixelbandito Tested it locally and bumped the version, looking good 🙂