[ASNetworkImageNode] Assert if both URL and image are set

[garrettmoon]

Here's one other idea for addressing this problem. Essentially,
we assert if you've set both the URL and the image. I've played around
with Pinterest and there's only one case where we hit this (the transition).

So I've also added another method (which is a bummer, it's weird I know)
but there's one good reason to add this method, ephemeralImage, which is
the user doesn't have to manually clear it out like they would if they
used defaultImage to save memory.

Putting this up for discussion.

[garrettmoon]

Is this the cleanest way to make a write only property?

[maicki]

Yes it is

[garrettmoon]

@maicki definitely want your feedback on this.

[maicki]

I think it's an interesting idea, but I have the fear that this will add even more confusion. Now we have image, defaultImage,ephemeralImage and internally setters like _setImage: etc. our goal (for the future) should be to reduce the state and not add even more ...

[appleguy]

Here's my feedback, although I don't intend for it to be definitive — I'm interested in understanding what behavior @maicki would prefer we support.

1. The internal complexity of ASNetworkImageNode

• Is important to consider and aim to reduce. However, I believe it is essential that internal complexity not be a factor in the API design. The reason is that there are sufficiently many clients of ASDK (over 1,000 developers regularly interacting with it), so that some increase in internal complexity would suggest we should invest in a refactor rather than affect the API.

2. There are exactly 3 cases to consider ASNetworkImageNode with:

Note that in the last case (both are set), we must provide identical behavior regardless of call order — therefore, regardless of implementation complexity, the decision on API behavior is only for the "both" case and doesn't care about the order.

a. Just .URL

This behavior is simple. It should not change from the prior implementation. If externally read, the .image property will become set after the URL has loaded. Likewise, the rendered version will be cleared when exiting the Display range, and the .image property itself will be cleared existing the Preload range.

b. Just .image

This behavior is also simple, but doesn't work correctly in 2.0. When just the .image is set and .URL is never set, the class should behave like ASImageNode. Why? Several reasons:

• many, many clients (including all over Pinterest) have already relied on this behavior. That's because it's intuitive and very frequently convenient for app developers.
• the convenience for devs is highlighted by the case of PIImageNode. It would be very difficult if we had to create a separate subclass and ensure even brand-new developers always use the base vs network class in the right situation.
• It would also make it more difficult for us to add features internally. E.g., a network-loaded button background, or even the future ASVideoNode that doesn't subclass ASImageNode — they would each have to carefully manage a Network vs Non-Network node to support both the direct-set asset / thumbnail as well as the URL-based option.

c. Both .URL and .image.

The case of BOTH .URL and .image set is less clear. However, the case is clarified by seeing there are only two reasons a user would do this:

• they want to load the URL image but are providing a cached or smaller version of the image to "prime" the view,
• the code is incorrect and inconsistent. I don't believe there is a use case where the .image should take precedence over the .URL, especially because there are memory risks to doing this (ASDK can't yield the memory, so it should only ever be used with small icon-like assets and these almost never have .URLs)

So lastly, this diff is a suggestion from @garrettmoon for how to make 2c. less ambiguous while still being VERY strict at preventing developers from mistakenly entering the "2c2" case. The idea is that this will behave functionally identical to "it's not allowed to set both URL and .image on a Network node", but provide users a technically-possible way to support the commonly-useful pattern of "priming" a network image node.

3. To make this call (which would increase impl. complexity), let's look at the use case

What's causing this to be so important? It's transitions that upscale photos, including the exact case @rcancro is working on right now with Pin Closeup's standard zoom-in.

Por these, it is essential to support upscaling a low-res photo while immediately kicking off a new high-res download, as well as async-displaying the new version, as well as swapping in the high-res version as soon as it's done, in a seamless way. This behavior is super useful because it is so hard to handle at an application layer.

This was a very, very key use case that AsyncDisplayKit was architected to excel at in the pre-1.0 days, with ASMultiplexImageNode bearing the complexity of the use case. However, Multiplex has fallen into non-maintenance, and so we have two paths to choose at this point:

• Let Multiplex continue to fade away, and probably become deprecated (e.g. we don't need to spend any time on it). Support basic transition use case with something like "ephemeralImage", or simply using the .image + .URL combination as a simple implementation for this.
• Alternatively, plan to revive Multiplex and refactor it to share more code with Network. Simplify Multiplex API so the "common transition" use case can be done more easily than the current dataSource-based API. Keep NetworkImage as simple as possible and do not allow transitions to be performed with it.

Thanks for reading a long note, hope this was useful to the discussion :)

[maicki]

As already discussed extensively with @garrettmoon my thoughts around the behavior of each of the cases for an ASNetworkImageNode are:

a. Just .URL

Agree with @appleguy ... no changes: This behavior is simple. It should not change from the prior implementation. If externally read, the .image property will become set after the URL has loaded. Likewise, the rendered version will be cleared when exiting the Display range, and the .image property itself will be cleared existing the Preload range.

b. Just .image

Agree with @appleguy: This behavior is also simple, but doesn't work correctly in 2.0. When just the .image is set and .URL is never set, the class should behave like ASImageNode.

c. Both .URL and .image will be set

In the case that a client set's an URL and an image it's less clear, but the thinking is to reduce the state and the side effects it can have it should actually work this:

1. If a URL is set it behaves like an ASNetworkImageNode. If an image should show before the image did load from the network it should be assigned to the defaultImage property and not the image property directly. The same way as the ASNetworkImageNode works as of today. I don't see any reason why we should support setting an .image as temporary image directly if there is already the defaultImage ... that said defaultImage is a really poor name for what it actually is: a placeholder image before the image did load from the network.
2. As soon as a developer assigns an image (doesn't matter if an URL was set before) the node will be transformed to an ASImageNode and the old state will be discarded.
3. If an .image is set and a URL will be set the node will be transformed from an ASImageNode to an ASNetworkImageNode. The .image will be removed replaced with the defaultImage until the image did load from the network and replaces the defaultImage as image as the current ASNetworkImage does work.

Why this behavior?

Imho, the behavior above is clear to describe as well hopefully reduces side effects for all kinds of quirks that can happen and added complexity we have to implement in case we want to support setting and .image as well as an URL and if someone is crazy also setting a defaultImage at the same time.

Summary

To summarize the states:

1. Setting an URL, the node will become an ASNetworkImageNode and it works the same way as currently. If the developer would like to display an image before the final image did load from the network, it should use the defaultImage as it's currently the case. All previous state is discarded as soon as a new URL is set.
2. Setting an image directly to the image property, the node will become an ASImageNode with resetting all previous state like the defaultImage or the URL that was maybe set previously.

[garrettmoon]

Awesome discussion! So it sounds like we're all in agreement on adding the assert. And I've done a bunch of cleanup in Pinterest and essentially added the feature on our side to mimic ephemeralImage. So I'm going to pull that bit out of this PR. Thank you both for your feedback!

[Adlai-Holler]

@garrettmoon Sorry to bother, but could you rebase this when you have time and we'll land it?