fix(RCTImageLoader): Adds requiresMainQueueSetup to fix v0.52

[iRoachie]

Motivation

In react-native v0.52 this warning shows:

"RCTImageLoader requires main queue setup since it overrides `init` but doesn't implement `requiresMainQueueSetup`".

This removes the warning by implementing requiresMainQueueSetup on RCTImageLoader

Test Plan

• Create new react-native project
• Enable Remote JS Debugging
• Should see no warning regarding RCTImageLoader requiring main queue setup

Related Issues

#17504

Release Notes

[IOS] [BUGFIX] [Libraries/Image/RCTImageLoader.m] - Implements requiresMainQueueSetup

[pull-bot]

@facebook-github-bot label Android

Attention: @shergin

Generated by 🚫 dangerJS

[janicduplessis]

Does this actually require main queue setup? All the init does is settings an ivar to nil. Might be a case where we can set requiresMainQueueSetup to NO as an optimisation. cc @shergin

[shergin]

I agree with Janic @janicduplessis, but someone has to do the audit of this module. After one minute looking I think we should return NO but one minute is not enough.
I am totally not comfortable to make it main-threaded just because we don't know for sure.

[janicduplessis]

@shergin Do you know what the RedirectDelegate is? This isn't used at all in the repo so we can't really tell if it's safe or not without knowing the code that uses it. With the default init that sets it to nil it is safe for sure to init off the main queue.

[iRoachie]

Confirming still a warning in 0.53

[facebook-github-bot]

@iRoachie I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

[ide]

The assignment of the ivar should be able to run on any queue, whether or not the RCTImageRedirectProtocol implementations are thread-safe. The queues on which the redirect delegate is invoked are independent from requiresMainQueueSetup so it looks to me like we can correctly and safely return NO here.

[ide]

This should be NO unless there's some reason this isn't queue-safe.

[iRoachie]

@ide Is that a confirmation to change it to NO?

[ide]

I think it should be NO but would appreciate another analysis.

[hramos]

Looks like @janicduplessis@shergin and @ide are all on the same page -- this should return NO.

[iRoachie]

@hramos Updated

[facebook-github-bot]

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

[fkgozali]

Thanks for fixing this! Sorry for the delay in merging...

[jose920405]

Still happening in 0.56

[iRoachie]

This does not show in 0.56 for me

[jose920405]

[iRoachie]

@jose920405 None of your warnings refers to RCTImageLoader. They may come from other components within the react-native core or any native components you may be using in your project. You may want to create issues in the respective projects.