Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

iRoachie
Copy link
Contributor

@iRoachie iRoachie commented Jan 20, 2018 •
edited
Loading

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

@iRoachie iRoachie requested a review from shergin as a code owner January 20, 2018 07:40
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. cla signed labels Jan 20, 2018
@pull-bot
Copy link

@facebook-github-bot label Android

Attention: @shergin

Generated by 🚫 dangerJS

@janicduplessis
Copy link
Contributor

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
Copy link
Contributor

shergin commented Jan 25, 2018

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
Copy link
Contributor

@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
Copy link
Contributor Author

iRoachie commented Feb 6, 2018

Confirming still a warning in 0.53

@facebook-github-bot
Copy link
Contributor

@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
Copy link
Contributor

ide commented Feb 19, 2018

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
ide suggested changes Feb 21, 2018
View reviewed changes
Copy link
Contributor

@ide ide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@iRoachie
Copy link
Contributor Author

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

@ide
Copy link
Contributor

ide commented Feb 21, 2018

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

@sdwilsh sdwilsh removed the cla signed label Mar 1, 2018
@hramos
Copy link
Contributor

hramos commented Mar 5, 2018

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

@hramos hramos mentioned this pull request Mar 5, 2018
Closed
@iRoachie
fix(RCTImageLoader): Adds requiresMainQueueSetup to fix v0.52
e3d12d6 
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

Closes #17504
@iRoachie
Copy link
Contributor Author

iRoachie commented Mar 5, 2018

@hramos Updated

facebook-github-bot
facebook-github-bot reviewed Mar 5, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pentarex pentarex mentioned this pull request Mar 11, 2018
Open
@hramos hramos added the Platform: iOS iOS applications. label Mar 13, 2018
@react-native-bot react-native-bot added Bug Fix 🐛 Platform: iOS iOS applications. Ran Commands One of our bots successfully processed a command. labels Mar 16, 2018
@fkgozali
Copy link
Contributor

fkgozali commented Apr 3, 2018

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

@iRoachie iRoachie deleted the imageLoader branch April 4, 2018 00:16
campsafari pushed a commit to exozet/react-native that referenced this pull request Apr 11, 2018
@iRoachie @campsafari
fix(RCTImageLoader): Adds requiresMainQueueSetup to fix v0.52
99b9c7e 
Summary:
<!--
Thank you for sending the PR! We appreciate you spending the time to work on these changes.

Help us understand your motivation by explaining why you decided to make this change.

You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html

Happy contributing!

-->

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

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

<!--
Help reviewers and the release process by writing your own release notes

**INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

  CATEGORY
[----------]        TYPE
[ CLI      ]   [-------------]      LOCATION
[ DOCS     ]   [ BREAKING    ]   [-------------]
[ GENERAL  ]   [ BUGFIX      ]   [-{Component}-]
[ INTERNAL ]   [ ENHANCEMENT ]   [ {File}      ]
[ IOS      ]   [ FEATURE     ]   [ {Directory} ]   |-----------|
[ ANDROID  ]   [ MINOR       ]   [ {Framework} ] - | {Message} |
[----------]   [-------------]   [-------------]   |-----------|

[CATEGORY] [TYPE] [LOCATION] - MESSAGE

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
[IOS] [BUGFIX] [Libraries/Image/RCTImageLoader.m] - Implements `requiresMainQueueSetup`
Closes facebook#17679

Reviewed By: shergin

Differential Revision: D7159601

Pulled By: fkgozali

fbshipit-source-id: e17bae67f4005d2c9ddd0d3701506521f3cac152
LukeDurrant pushed a commit to LukeDurrant/react-native that referenced this pull request Apr 11, 2018
@iRoachie @LukeDurrant
fix(RCTImageLoader): Adds requiresMainQueueSetup to fix v0.52
71f2f05 
Summary:
<!--
Thank you for sending the PR! We appreciate you spending the time to work on these changes.

Help us understand your motivation by explaining why you decided to make this change.

You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html

Happy contributing!

-->

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

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

<!--
Help reviewers and the release process by writing your own release notes

**INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

  CATEGORY
[----------]        TYPE
[ CLI      ]   [-------------]      LOCATION
[ DOCS     ]   [ BREAKING    ]   [-------------]
[ GENERAL  ]   [ BUGFIX      ]   [-{Component}-]
[ INTERNAL ]   [ ENHANCEMENT ]   [ {File}      ]
[ IOS      ]   [ FEATURE     ]   [ {Directory} ]   |-----------|
[ ANDROID  ]   [ MINOR       ]   [ {Framework} ] - | {Message} |
[----------]   [-------------]   [-------------]   |-----------|

[CATEGORY] [TYPE] [LOCATION] - MESSAGE

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
[IOS] [BUGFIX] [Libraries/Image/RCTImageLoader.m] - Implements `requiresMainQueueSetup`
Closes facebook#17679

Reviewed By: shergin

Differential Revision: D7159601

Pulled By: fkgozali

fbshipit-source-id: e17bae67f4005d2c9ddd0d3701506521f3cac152
LukeDurrant pushed a commit to LukeDurrant/react-native that referenced this pull request Apr 11, 2018
@iRoachie @LukeDurrant
fix(RCTImageLoader): Adds requiresMainQueueSetup to fix v0.52
7226766 
Summary:
<!--
Thank you for sending the PR! We appreciate you spending the time to work on these changes.

Help us understand your motivation by explaining why you decided to make this change.

You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html

Happy contributing!

-->

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

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

<!--
Help reviewers and the release process by writing your own release notes

**INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

  CATEGORY
[----------]        TYPE
[ CLI      ]   [-------------]      LOCATION
[ DOCS     ]   [ BREAKING    ]   [-------------]
[ GENERAL  ]   [ BUGFIX      ]   [-{Component}-]
[ INTERNAL ]   [ ENHANCEMENT ]   [ {File}      ]
[ IOS      ]   [ FEATURE     ]   [ {Directory} ]   |-----------|
[ ANDROID  ]   [ MINOR       ]   [ {Framework} ] - | {Message} |
[----------]   [-------------]   [-------------]   |-----------|

[CATEGORY] [TYPE] [LOCATION] - MESSAGE

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
[IOS] [BUGFIX] [Libraries/Image/RCTImageLoader.m] - Implements `requiresMainQueueSetup`
Closes facebook#17679

Reviewed By: shergin

Differential Revision: D7159601

Pulled By: fkgozali

fbshipit-source-id: e17bae67f4005d2c9ddd0d3701506521f3cac152
bunnyc1986 pushed a commit to bunnyc1986/react-native that referenced this pull request May 11, 2018
@iRoachie @bunnyc1986
fix(RCTImageLoader): Adds requiresMainQueueSetup to fix v0.52
d37fb02 
Summary:
<!--
Thank you for sending the PR! We appreciate you spending the time to work on these changes.

Help us understand your motivation by explaining why you decided to make this change.

You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html

Happy contributing!

-->

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

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

<!--
Help reviewers and the release process by writing your own release notes

**INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

  CATEGORY
[----------]        TYPE
[ CLI      ]   [-------------]      LOCATION
[ DOCS     ]   [ BREAKING    ]   [-------------]
[ GENERAL  ]   [ BUGFIX      ]   [-{Component}-]
[ INTERNAL ]   [ ENHANCEMENT ]   [ {File}      ]
[ IOS      ]   [ FEATURE     ]   [ {Directory} ]   |-----------|
[ ANDROID  ]   [ MINOR       ]   [ {Framework} ] - | {Message} |
[----------]   [-------------]   [-------------]   |-----------|

[CATEGORY] [TYPE] [LOCATION] - MESSAGE

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
[IOS] [BUGFIX] [Libraries/Image/RCTImageLoader.m] - Implements `requiresMainQueueSetup`
Closes facebook#17679

Reviewed By: shergin

Differential Revision: D7159601

Pulled By: fkgozali

fbshipit-source-id: e17bae67f4005d2c9ddd0d3701506521f3cac152
@jozan jozan mentioned this pull request May 29, 2018
Closed
@benjarwar benjarwar mentioned this pull request Jun 14, 2018
Closed
macdoum1 pushed a commit to macdoum1/react-native that referenced this pull request Jun 28, 2018
@iRoachie @macdoum1
fix(RCTImageLoader): Adds requiresMainQueueSetup to fix v0.52
58dee5f 
Summary:
<!--
Thank you for sending the PR! We appreciate you spending the time to work on these changes.

Help us understand your motivation by explaining why you decided to make this change.

You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html

Happy contributing!

-->

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

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

<!--
Help reviewers and the release process by writing your own release notes

**INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

  CATEGORY
[----------]        TYPE
[ CLI      ]   [-------------]      LOCATION
[ DOCS     ]   [ BREAKING    ]   [-------------]
[ GENERAL  ]   [ BUGFIX      ]   [-{Component}-]
[ INTERNAL ]   [ ENHANCEMENT ]   [ {File}      ]
[ IOS      ]   [ FEATURE     ]   [ {Directory} ]   |-----------|
[ ANDROID  ]   [ MINOR       ]   [ {Framework} ] - | {Message} |
[----------]   [-------------]   [-------------]   |-----------|

[CATEGORY] [TYPE] [LOCATION] - MESSAGE

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
[IOS] [BUGFIX] [Libraries/Image/RCTImageLoader.m] - Implements `requiresMainQueueSetup`
Closes facebook#17679

Reviewed By: shergin

Differential Revision: D7159601

Pulled By: fkgozali

fbshipit-source-id: e17bae67f4005d2c9ddd0d3701506521f3cac152
@fungilation fungilation mentioned this pull request Jun 29, 2018
Closed
@jose920405
Copy link

Still happening in 0.56

@iRoachie
Copy link
Contributor Author

This does not show in 0.56 for me

@jose920405
Copy link

captura de pantalla 2018-08-17 a la s 6 10 25 pm

@iRoachie
Copy link
Contributor Author

@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.

dthian pushed a commit to viromedia/viro that referenced this pull request Sep 25, 2019
@manbod
[VIRO-4783] Fix for iOS bridge showing 4 (requires main queue setup)w…
81c55f2 
…arnings

This change mimics changes made in other RN modules like RCTImageLoader, react-native-share,etc
react-native-share/react-native-share#331
facebook/react-native#17679

Starting this RN version, FB seems to default to NO main queue anyways, if not specified.
But they raise these warnings anyway.


Former-commit-id: 01a4cb4fed3760290f90d613e74ffa9a6b18c981
ktemby pushed a commit to ktemby/viro that referenced this pull request Oct 4, 2020
@manbod
[VIRO-4783] Fix for iOS bridge showing 4 (requires main queue setup)w…
5db5852 
…arnings

This change mimics changes made in other RN modules like RCTImageLoader, react-native-share,etc
react-native-share/react-native-share#331
facebook/react-native#17679

Starting this RN version, FB seems to default to NO main queue anyways, if not specified.
But they raise these warnings anyway.


Former-commit-id: 01a4cb4fed3760290f90d613e74ffa9a6b18c981
Former-commit-id: 7f7b2f4 [formerly 81c55f2]
Former-commit-id: 603678e10722369faebb9116903a9a96e809dfb6
ktemby pushed a commit to ktemby/viro that referenced this pull request May 3, 2021
@manbod
[VIRO-4783] Fix for iOS bridge showing 4 (requires main queue setup)w…
7f7b2f4 
…arnings

This change mimics changes made in other RN modules like RCTImageLoader, react-native-share,etc
react-native-share/react-native-share#331
facebook/react-native#17679

Starting this RN version, FB seems to default to NO main queue anyways, if not specified.
But they raise these warnings anyway.


Former-commit-id: 01a4cb4fed3760290f90d613e74ffa9a6b18c981
Former-commit-id: 81c55f2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@ide ide ide requested changes

@shergin shergin Awaiting requested review from shergin

Assignees
No one assigned
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: iOS iOS applications. Ran Commands One of our bots successfully processed a command.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.