Fix Xcode warnings in React-Core pod #29622
Conversation
facebook-github-bot
added
the
CLA Signed
Base commit: f8e5423 |
|
cc @alloy |
Base commit: f8e5423 |
.gitignore
Outdated
| @@ -88,6 +88,7 @@ RNTester/build | |||
| /template/ios/Pods/ | |||
| /template/ios/Podfile.lock | |||
| /RNTester/Gemfile.lock | |||
| /RNTester/vendor | |||
There was a problem hiding this comment.
The vendor directory came for running bundle install --path vendor, which I did to avoid installing the gems as system gems. I can remove it from this PR. It would be nice to have some better instructions on how to set up the development environment for newbies.
React/Base/RCTBridge.m
Outdated
| [self reloadWithReason:@"Command"]; | ||
| #pragma clang diagnostic pop |
There was a problem hiding this comment.
Do you know if this is being tracked in an issue? Seeing as it will now go silent.
There was a problem hiding this comment.
Thanks for calling this out. I went ahead and took a stab at moving off these deprecated methods. Let me know if I should revert that and just file an issue instead.
React/Base/RCTJSStackFrame.m
Outdated
| @@ -100,7 +100,7 @@ + (instancetype)stackFrameWithLine:(NSString *)line | |||
| file:file | |||
| lineNumber:[lineNumber integerValue] | |||
| column:[column integerValue] | |||
| collapse:@NO]; | |||
| collapse:@NO.integerValue]; | |||
There was a problem hiding this comment.
This seems a smell. Can the collapse var be made a BOOL instead, at least internally to this class? If not, then at least it seems better to just pass 0.
There was a problem hiding this comment.
Agreed. Shouldn't be a huge change.
There was a problem hiding this comment.
I changed it to BOOL.
React/CxxBridge/RCTCxxBridge.mm
Outdated
| @@ -1125,6 +1128,8 @@ - (void)reloadWithReason:(NSString *)reason | |||
| RCTTriggerReloadCommandListeners(reason); | |||
| } | |||
|
|
|||
| #pragma clang diagnostic pop | |||
There was a problem hiding this comment.
Same as before, this should be tracked in some way. Perhaps the alternative to an issue is to leave comments pointing to these call-sites from the deprecated implementation?
There was a problem hiding this comment.
Since RCTCxxBridge inherits from RCTBridge, I went ahead and just removed these overrides, since RCTBridge now has the same implementation as this (minus the warn log).
React/Modules/RCTUIManagerUtils.m
Outdated
| @@ -8,6 +8,7 @@ | |||
| #import "RCTUIManagerUtils.h" | |||
|
|
|||
| #import <libkern/OSAtomic.h> | |||
| @@ -98,6 +99,6 @@ void RCTUnsafeExecuteOnUIManagerQueueSync(dispatch_block_t block) | |||
| NSNumber *RCTAllocateRootViewTag() | |||
| { | |||
| // Numbering of these tags goes from 1, 11, 21, 31, ..., 100501, ... | |||
| static int64_t rootViewTagCounter = -1; | |||
| return @(OSAtomicIncrement64(&rootViewTagCounter) * 10 + 1); | |||
| static _Atomic int64_t rootViewTagCounter = 0; | |||
There was a problem hiding this comment.
From the docs:
Atomically replaces the value pointed by obj with the result of addition of arg to the old value of obj, and returns the value obj held previously.
So that’s why this should initialise with 0 instead of -1, like previously was the case.
Do you know if there are tests that cover this API?
There was a problem hiding this comment.
I don't know of any tests per se, but the RNTester app crashed immediately when this was initialized to -1.
React/Profiler/RCTProfile.m
Outdated
| [RCTProfilingBridge() reloadWithReason:@"Profiling controls"]; | ||
| #pragma clang diagnostic pop |
jdeff
force-pushed
the
fix-xcode-warnings
branch
from
8f6d0c8
to
694f056
Compare
|
Thanks for the review @alloy. I think I've resolved all of the comments. |
There was a problem hiding this comment.
@appden has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
This pull request was successfully merged by Jayme Deffenbaugh in cb719a1. When will my fix make it into a release? | Upcoming Releases |
| @@ -36,6 +36,9 @@ NS_ASSUME_NONNULL_BEGIN | |||
| @property (nonatomic, assign) NSTimeInterval loadingViewFadeDelay; | |||
| @property (nonatomic, assign) NSTimeInterval loadingViewFadeDuration; | |||
|
|
|||
| - (instancetype)initWithSurface:(RCTSurface *)surface | |||
| sizeMeasureMode:(RCTSurfaceSizeMeasureMode)sizeMeasureMode NS_UNAVAILABLE; | |||
Summary
With the upgrade to React Native 0.63, we started running into nullability warnings that were breaking our build. This PR fixes those nullability warnings as well as a few other warnings in React-Core.
Changelog
[iOS] [Fixed] - Fix xcodebuild warnings in React-Core
Test Plan
RCTAllocateRootViewTagis the only real logic change in this PR. We throw an exception if the root view tag is not in the correct format, so this change seems safe after some basic manual testing in RNTester.