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.

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

Version 0.40.0 gives 12 issues in xcode #11736

Closed
almadsen opened this issue Jan 5, 2017 · 25 comments
Closed

Version 0.40.0 gives 12 issues in xcode #11736

almadsen opened this issue Jan 5, 2017 · 25 comments
Labels
Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Help Wanted :octocat: Issues ideal for external contributors. Platform: iOS iOS applications. Resolution: Locked This issue was locked by the bot.

Comments

@almadsen
Copy link

almadsen commented Jan 5, 2017

Straight from init a new project and opening it with Xcode and running it in the iOS simulator I get 12 issues in Xcode, and this is without changing any code.

Did not get any issues with version 0.39.0

skarmavbild 2017-01-05 kl 17 28 22

––––––––––––––––––––––––––––––

  • React Native version: 0.40.0
  • Platform: iOS
  • Operating System: MacOS
@almadsen almadsen changed the title Version 0.40.0 gives 13 issues in xcode Version 0.40.0 gives 12 issues in xcode Jan 5, 2017
@andreencar
Copy link

Maybe some modules are affected by this?
https://github.com/facebook/react-native/releases/tag/v0.40.0

@almadsen
Copy link
Author

almadsen commented Jan 5, 2017

No modules, I get this straight from initializing the project, so it's clean. No code changed, no modules, nothing. This is the files originally created.

@chirag04
Copy link
Contributor

chirag04 commented Jan 5, 2017

Those are warning and you can mostly ignore those warning. You project should run fine with those warning.

@janicduplessis
Copy link
Contributor

It would be great if someone can send a PR to fix those warning, from the screenshot it seems pretty straightforward.

@janicduplessis janicduplessis added Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Help Wanted :octocat: Issues ideal for external contributors. Platform: iOS iOS applications. labels Jan 5, 2017
@LiuC520
Copy link

LiuC520 commented Jan 8, 2017

also these problems? when i use 0.40, some third packages are wrong

@neilsarkar
Copy link
Contributor

For the RCTExecuteOnMainThread warnings, the deprecation warning states:

RCT_EXTERN void RCTExecuteOnMainThread(dispatch_block_t block, BOOL sync)
__deprecated_msg("Use RCTExecuteOnMainQueue instead. RCTExecuteOnMainQueue is "
                 "async. If you need to use the `sync` option... please don't.");

the two uses that are generating warnings are in RCTUtils.m and are low level functions for getting screen scale and size -- they both have many callers that expect a synchronous method signature.

// RCTUtils.m

CGFloat RCTScreenScale()
{
  static CGFloat scale;
  static dispatch_once_t onceToken;
  dispatch_once(&onceToken, ^{
    RCTExecuteOnMainThread(^{
      scale = [UIScreen mainScreen].scale;
    }, YES);
  });

  return scale;
}

CGSize RCTScreenSize()
{
  // FIXME: this caches the bounds at app start, whatever those were, and then
  // doesn't update when the device is rotated. We need to find another thread-
  // safe way to get the screen size.

  static CGSize size;
  static dispatch_once_t onceToken;
  dispatch_once(&onceToken, ^{
    RCTExecuteOnMainThread(^{
      size = [UIScreen mainScreen].bounds.size;
    }, YES);
  });

  return size;
}

I understand why we don't want to make synchronous calls on the main thread. Is this an exception? Would the preferred approach be to dispatch directly from these two functions instead of using the deprecated function? Or is the solution to refactor all callers to handle an asynchronous method signature?

@janicduplessis
Copy link
Contributor

In this case we might want to silence the warning since there isn't an alternative to this.

@janicduplessis
Copy link
Contributor

Another option (maybe better) would be to create RCTExecuteOnMainQueueSync that does what RCTExecuteOnMainThread(block, YES) does. RCTExecuteOnMainQueueSync could have a comment that says you shouldn't use it unless you know what you are doing.

@neilsarkar
Copy link
Contributor

Thanks for the info @janicduplessis.

It's 2am in Portugal so I'm gonna call it for tonight but will implement this suggestion and submit tomorrow, although I'm not sure the issue will still be there by then judging by the trajectory @rh389 is on with these warnings :)

@robhogan
Copy link
Contributor

Haha I'm also about to call it a night (UK) - I'll leave the RCTExecuteOnMainThread warnings to you as I was unsure how to handle those myself. The event dispatcher one is also tricky since switching to a scoped event would be a BC.

Some progress though - if these go through it's 8 down, 4 to go.
#11797
#11798
#11799

@robhogan
Copy link
Contributor

#11806 and #11807 just leaves the RCTExecuteOnMainThread and sendDeviceEventWIthName deprecation warnings.

I'm going to take a break for now and do something I'm paid for ;). Will revisit this in a day or two if there's still anything outstanding.

@janicduplessis
Copy link
Contributor

Thanks a lot for working on this :)

facebook-github-bot pushed a commit that referenced this issue Jan 11, 2017
Summary:
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.
Closes #11817

Differential Revision: D4402911

fbshipit-source-id: 9fd8b3f50d34984b765fe22b1f4512e103ba55a9
facebook-github-bot pushed a commit that referenced this issue 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 issue 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
@almadsen
Copy link
Author

almadsen commented Feb 4, 2017

Ok so in latest release, 0.41 warnings are still there.
So basically my question now is if it's something I should worry about, or should I just try to mute them out?

@janicduplessis
Copy link
Contributor

janicduplessis commented Feb 4, 2017

Most of them are fixed in master, I think these commits didn't make it to 0.41 sadly. You can just ignore them.

@robhogan
Copy link
Contributor

robhogan commented Feb 4, 2017

@janicduplessis I'd appreciate your thoughts on the last one remaining (last I checked). sendDeviceEventWithName is deprecated, apparently RCTUIManager should switch to module-scoped events instead. That in itself would be a small BC but probably acceptable.

The messy part is that this would diverge from the way Android emits the same events. AFAIK Android has no similar mechanism for module emitters and everything's global there. So the listening side, Dimensions , for example, would have to fork by platform, and must be aware of UIManager

If global events are genuinely deprecated (ie, will be removed) in iOS then this is something we've just got to do, and hopefully Android will catch up some day. If global events are going to hang around for a while yet, we might be better suppressing this warning until there's an Android equivalent. Or do we just take the plunge now and accept a bit of inconsistency?

@joshjhargreaves
Copy link
Contributor

Hey, @rh389, @janicduplessis does this seem sensible?

@joemckie
Copy link

joemckie commented Apr 8, 2017

@janicduplessis I'm still getting very similar errors in 0.43.2. Is this still a valid issue?

screen shot 2017-04-08 at 09 03 25

@ndbroadbent
Copy link

ndbroadbent commented Apr 19, 2017

@janicduplessis If I sent a pull request to remove all of these deprecation warnings, how long might it take for that to be accepted?

(I noticed there's an enormous number of issues and pull requests, so I'm worried about wasting my time if it never gets any attention.)

@robhogan
Copy link
Contributor

I think the only outstanding deprecation already has a PR #12209.

Many of the unused param warnings only appear in release builds because they're due to debug checks that get preprocessed away. I guess it should be possible to suppress that warning via a compiler setting for release only.

@ndbroadbent
Copy link

@rh389 Ah great, thanks.

Yeah that would be great if they could be suppressed. There's a lot of noise when I build the project in Xcode, and I have to scroll down a long way to see the actual errors.

@joshjhargreaves
Copy link
Contributor

@ndbroadbent, you can actually filter to show only errors and not warnings in XCode.

screen shot 2017-04-20 at 10 15 59 am

It's the second icon on the right.

@hramos
Copy link
Contributor

hramos commented Jul 20, 2017

Hi there! This issue is being closed because it has been inactive for a while. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. Either way, we're automatically closing issues after a period of inactivity. Please do not take it personally!

If you think this issue should definitely remain open, please let us know. The following information is helpful when it comes to determining if the issue should be re-opened:

  • Does the issue still reproduce on the latest release candidate? Post a comment with the version you tested.
  • If so, is there any information missing from the bug report? Post a comment with all the information required by the issue template.
  • Is there a pull request that addresses this issue? Post a comment with the PR number so we can follow up.

If you would like to work on a patch to fix the issue, contributions are very welcome! Read through the contribution guide, and feel free to hop into #react-native if you need help planning your contribution.

@hramos hramos added the Icebox label Jul 20, 2017
@hramos hramos closed this as completed Jul 20, 2017
@leonardpauli
Copy link

leonardpauli commented Sep 6, 2017

This issue is still valid, actually, I got closer to 100 warnings and a couple of "Logic errors" - from a fresh install! Those are noisy when trying to develop native components.

@rh389 said many where due to automatic release pre-processing. To suppress these warning - ONLY ON THE REACT STUFF - ie. not on your own code; change build settings for the relevant targets:

image

https://stackoverflow.com/questions/7997636/disable-warnings-in-xcode-from-frameworks

for the logic errors; see "-Xanalyzer -analyzer-disable-all-checks"

@vynogradskyi
Copy link

Confirm this issue still appears on React-Native "0.55.4". 170 warnings
screen shot 2018-06-13 at 13 28 29

@facebook facebook locked and limited conversation to collaborators Jun 13, 2018
@hramos
Copy link
Contributor

hramos commented Jun 13, 2018

Please give 0.56.0-rc a try - this is specifically called out in the changelog.

@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Help Wanted :octocat: Issues ideal for external contributors. Platform: iOS iOS applications. Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests