Fix Image defaultSource not showing on iOS

[cristianoccazinsp]

Summary

Fix Image defaultSource not showing on iOS.

This bug was introduced somewhere between RN 0.63 and 0.65. On iOS, defaultSource does not show while the image is being downloaded, only if it fails or there's no internet.

Changelog

[iOS] [Fixed] - Fix Image defaultSource not showing on iOS

Test Plan

Ran both debug and release builds on an iPhone 12 pro (iOS 14.6)

[facebook-github-bot]

Hi @cristianoccazinsp!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: d4c347c

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,959,192	+2,226
android	hermes	armeabi-v7a	8,490,569	+2,254
android	hermes	x86	9,379,985	+1,754
android	hermes	x86_64	9,344,467	+2,084
android	jsc	arm64-v8a	10,641,430	+2,559
android	jsc	armeabi-v7a	9,560,330	+2,580
android	jsc	x86	10,656,516	+2,089
android	jsc	x86_64	11,264,674	+2,399

Base commit: d4c347c

[facebook-github-bot]

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[lunaleaps]

@cristianoccazinsp Do you think you could also put together a RNTester example of this behavior to ensure we don't break it again? We can add some E2E testing around it internally

[cristianoccazinsp]

@lunaleaps this is my first PR ever to RN so I wouldn't even know where to start! In fact, I'm not 100% sure this is the right place to fix this issue. When debugging, the issue is clear, the default image is never loaded by default unless there's a network error, so the fix made sense in this location. However, who ever changed this last may have more idea if this is the right place to add the "load default image" code.

Either way, if you could point me where to start with a RNTester change, I could try.

[lunaleaps]

Gotcha

Yea basically you want to create a reproduction of the issue you were seeing and stick it as an example in here:

react-native/packages/rn-tester/js/examples/Image/ImageExample.js

Line 243 in 1465c8f

<Image

Have you run RNTester before? Here are the instructions to run in this repo: https://github.com/facebook/react-native/tree/1465c8f3874cdee8c325ab4a4916fda0b3e43bdb/packages/rn-tester#running-this-app

Once you have that app running, you can navigate to the Image examples.
I'd imagine you'd want to construct a way to reproduce the issue.. how did you test it for yourself?

[cristianoccazinsp]

The test was simple, defaultSource simply does not work on iOS and RN 0.65.1; it doesn’t work at all unless you run the app without internet connectivity.

It also has the caveat that if you must run this in release mode as without internet not even the default source image will work (because resources are downloaded from the host when in debug mode)

[lunaleaps]

@cristianoccazinsp Got it, then maybe just adding an image with a default source is sufficient to RNTester.

[cristianoccazinsp]

I will give it a try once I get some free time, although most likely out of scope for me.

Any chance someone with more knowledge of this code can take a look as well to make sure I changed what I was supposed to?

[cristianoccazinsp]

@lunaleaps I made the effort to test the rn-tester app image section. I believe images with defaultSource where there, even if they didn't work. How are you even testing this?

I made a slight tweak to the test so it loads a larger image and also busts cache in order to ensure the defaultSource image is in fact visible.

I have also confirmed that my PR fixes the issue, and it was otherwise broken without the native iOS change. However, I don't know how are you going to ensure this is tested and detected if it breaks again.

Lastly, and perhaps material for another PR, there are various unhandled promise rejection logs and warnings, the code doesn't seem to be bug-free.

[cristianoccazinsp]

@lunaleaps any updates on this? Think this can make its way up to the next RN version? What's missing?

Thanks!

[facebook-github-bot]

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[cristianoccazinsp]

Does this mean it was included in a following release, or just ignored?

[lunaleaps]

@cristianoccazinsp it was commited in 900210c and it will be included in the next release.