Remove RCTBridgeWillInvalidateModulesNotification observer from RCTDeviceInfo #45012
Conversation
disable codesigning for tests
|
Hi @hromovp! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
facebook-github-bot
added
CLA Signed
Base commit: 9491ded |
hromovp
changed the title
There was a problem hiding this comment.
Hi @hromovp, thanks for the contribution.
I don't think we can proceed with this, given that we needed to listen to that event to make sure that RCTDeviceInfo is invalidated.
Calling the event twice, in the tear-down step should be idempotent, so it should not cause any issue.
Out of curiosity: which configuration are you using?
Are you on the new architecture? Are you using bridgeless or not?
| @@ -80,7 +80,9 @@ runTests() { | |||
| -scheme RNTester \ | |||
| -sdk iphonesimulator \ | |||
| -destination "platform=iOS Simulator,name=$IOS_DEVICE,OS=$IOS_TARGET_OS" \ | |||
| "${SKIPPED_TESTS[@]}" | |||
There was a problem hiding this comment.
Can you do this in a separate PR, please?
| [[NSNotificationCenter defaultCenter] addObserver:self | ||
| selector:@selector(invalidate) | ||
| name:RCTBridgeWillInvalidateModulesNotification |
There was a problem hiding this comment.
We can't really remove this.
If you look at the change that introduced this, it solves another bug: #42120.
|
Hi @cipolleschi, I am using 0.74.2 with bridge. I am fully aware of the issue this was resolving and therefore I have only removed notification that is, to my understanding, redundant. Despite that, I understand your position and tbh main reason for creating this PR was to notify RN team that there could be issue with this. |
|
The New Architecture with Bridge is not the suggested setup anymore, and we will probably not want to fix it. If this problem is happening in the Old Architecture, then we need to look into it. Which one of the two scenarios are you into? |
Summary:
I noticed that after application reload useWindowDimensions stops listening to dimension changes on app orientation change, width and height had always the same value regardles of orientation.
On further investigation it became clear that invalidating method were running twice on turndown, which led me to believe that turbomodule already invalidating imperatively (as stated in the comment by author of workaround) and calling notification handler invalidates newly created module, as a result nothing handles screen size changes.
Also I removed codesigning for running tests as its requesting development team and failing.
Changelog:
[IOS] [REMOVED] - Remove RCTBridgeWillInvalidateModulesNotification observer from RCTDeviceInfo
Test Plan
invalidateandinitializemethods ofreact-native/packages/react-native/React/CoreModules/RCTDeviceInfo.mm;current state: on reload
invalidatemethod runs twice, removing observers for both previous and new instances of RCTDeviceInfo class, you can check by memory adress of the instance on intiallize and invalidate callsexpected: on reload
invalidatemethod runs once removing previous instance of the class, every subsequent reload it does same.