Skip to content

[iOS build warnings] Remove deprecation warning for RCTExecuteOnMainThread #11817

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.

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

Closed

Conversation

neilsarkar
Copy link
Contributor

As per @janicduplessis recommendation, provide a new synchronous method to replace the necessary synchronous calls and use a warning in the comments (and method name).

Motivation

There are currently a number of XCode warnings that show up in a fresh 0.40 install of a react native project. While the project can still be run, this contributes negatively to the development experience -- valid warnings may be ignored and new ones may be added as per https://en.wikipedia.org/wiki/Broken_windows_theory

This addresses one of the warnings, by providing the functionality of a deprecated method in two specific cases where we can't avoid doing synchronous work on the main queue. See #11736 for more context.

Test plan (required)

I ran a project that relied on screen size and it didn't crash...happy to do more involved testing if someone can share better methodology.

As per @janicduplessis recommendation, provide a new synchronous method to replace the necessary synchronous calls and use a warning in the comments (and method name).
@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @javache and @nicklockwood to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jan 10, 2017
@neilsarkar neilsarkar changed the title [iOS warnings] Remove deprecation warning for RCTExecuteOnMainThread [iOS build warnings] Remove deprecation warning for RCTExecuteOnMainThread Jan 10, 2017
@ide
Copy link
Contributor

ide commented Jan 11, 2017

Looks good, thanks for doing this. Would you be interested in sending a followup PR that deletes the deprecated method?

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

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

@neilsarkar neilsarkar deleted the neil.ios-warnings branch January 11, 2017 15:55
@neilsarkar
Copy link
Contributor Author

@ide looks like there are a few third party components that still rely on this https://github.com/search?p=1&q=RCTExecuteOnMainThread&type=Code&utf8=✓

How long do things generally stay deprecated for before being removed? Happy to remove it for 0.41 if you think that's enough warning for people, although it should be mentioned in release notes as well, let me know how to indicate that.

@ide
Copy link
Contributor

ide commented Jan 11, 2017

This would ship with 0.42 and we'd announce the breaking in the release notes when the RC is cut (in about 3 weeks). Then 0.42 final would ship a month after that, so in total people have about 7 weeks of advance notice if they're following master (which most people don't) and a month's notice from the RC's release notes.

@janicduplessis
Copy link
Contributor

Looks like most of the results are RN forks so it shouldn't be too bad.

facebook-github-bot pushed a commit that referenced this pull request Jan 16, 2017
Summary:
**Motivation**

This finishes the job of #11817, removing the previously deprecated method. See #11736 for more context.

**Test plan**

There were no tests for this method, and I searched the whole project to make sure we weren't relying on it anywhere.
Closes #11854

Differential Revision: D4421671

Pulled By: javache

fbshipit-source-id: 67e0db8d3c594ad3ccd6ccdae08f8ce49ddb8a34
nicktate pushed a commit to nicktate/react-native that referenced this pull request Jan 19, 2017
Summary:
**Motivation**

This finishes the job of facebook#11817, removing the previously deprecated method. See facebook#11736 for more context.

**Test plan**

There were no tests for this method, and I searched the whole project to make sure we weren't relying on it anywhere.
Closes facebook#11854

Differential Revision: D4421671

Pulled By: javache

fbshipit-source-id: 67e0db8d3c594ad3ccd6ccdae08f8ce49ddb8a34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants