[Proof-of-concept] Image render API

[erickok]

A proposal for an all-encompassing image render API. See #497 for a discussion and #504 for an alternative.

[ryan-berger]

Overall, nice job, one small nitpick, but I would be fine accepting this proposal.

We will need to document this extremely well though, so before we merge we may want to do that....

[erickok]

I will complete my proposal with support for the other source/target combinations we currently supported.

[erickok]

Okay I am feature-complete now with regards to the existing implementation. As I bonus we support asset svgs. It's easy to extend support but the PR is big enough as it stands. @tneotia could you test-drive this for me?

[tneotia]

@erickok Test-drive has passed with flying colors. Just three things:

1. If I use

assetUriMatcher(): (context, attributes, element) {
              return Image.asset(attributes['src'].replaceAll("asset:", ""));
            }

I get an error because the SVG element can't be rendered by Image.asset. I created a review above that talked about this, basically I think that you should split the asset rendering into an assetImage render and an assetSvg render, like how you've split the network renders.

2. I created another review above related to the question I had on Use intrinsic with for <table> columns with otherwise undefined size #519 where you said that we no longer had to check for the size of the image before passing to a table, please check that.

3. There seems to be a merge conflict, can you update your branch and push again to resolve it?

Great work, this is fantastic! Can't wait for the customRender to transition to this type of implementation too.

P.S. Go to sleep - you're working on this at like 2a.m. lol No need to be grinding that much :)

[ryan-berger]

@erickok Could we also add some unit tests for matchers before we merge? I think that this is a thing that ought to have tests just in case

[erickok]

@ryan-berger I have added unit tests for the different image source matchers (asset, base64 data and network).

@tneotia good point, I have included the fix you did for #523 and made sure the size checking (for use in tables) is still needed and unfortunately it still is (so it is working as intended now).

[erickok]

[ryan-berger]

@erickok This is looking very good, I think I have one last question that I'm not sure if we addressed via proposals:

Who is handing the error if the render function for the image throws an exception? Do we want to throw in a placeholder?

Once we have that answered, I say we merge it

[erickok]

Good point. For the moment I decided not to change this behaviour, so the error callback is still as-is, namely the onImageError. That's for 'expected' errors such as when an image could not be loaded. Programming errors are caught in any specific way (and we don't do that anywhere else either in our lib).

[ryan-berger]

Oh duh, forgot about that callback.

I think this looks good to me then! I'll approve the merge and let you do it!

[ryan-berger]

Nicely done! I'm quite excited to see this go to production!

[erickok]

Thanks for the help and support, @ryan-berger and @tneotia . I am going to merge. Much more coming soon!