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[AppContainer]: mount react devtools overlay only when devtools are attached #38785

Merged
merged 1 commit into from
Aug 7, 2023
Merged

fix[AppContainer]: mount react devtools overlay only when devtools are attached #38785

merged 1 commit into from
Aug 7, 2023

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented Aug 4, 2023

Cherry-picking #38727 into 0.72

@hoxyq
fix[AppContainer]: mount react devtools overlay only when devtools ar…
ea3efa8 
…e connected (facebook#38727)

Summary:
Pull Request resolved: facebook#38727

## Changelog:
[General][Fix] - Do not render React DevTools overlays unless they are connected

Fixes facebook#38024.
Reproducible example: https://github.com/rasaha91/rn-bottomsheet.

- This is a temporary workaround to resolve described problem for DEV bundles without attached React DevTools.
- Still, such problem will be present for DEV bundles with attached React DevTools, but this should be only for brownfield apps with a shrinked React Native window.

Checking that DevTools hook is present is not enough to determine whether the DevTools are connected. These changes fix it.

Short description of what's going on there:
1. When React Native root window is rendered inside some native view, which takes only portion of the screen (like the native bottom sheet in the reproducible example), DevtoolsOverlay / InspectorOverlay takes up space based on application window dimension, this results into resizing the hosting window on the native side.
https://pxl.cl/357r3
2. Right way to fix this would be removing the usage of application window sizes, so that DevtoolsOverlay / InspectorOverlay will be allowed only to take React Native's window.
3. Unfortunately, just removing setting is not enough, we should also have at least 1 of 2 things:
- `collapsable` prop should be set to `true` => View will be flattened
- Remove [`flex: 1` style on both root and inner Views](https://github.com/facebook/react-native/blob/b28e3c16ed7cbc8b3ed3f26d91c58acb4bb28879/packages/react-native/Libraries/ReactNative/AppContainer.js#L145-L147), but this is breaking how LogBox works now.
| {F1062478964} |  {F1062492367}

Reviewed By: NickGerleman

Differential Revision: D47954883

fbshipit-source-id: d45d8cb82daa8dc9de58f54c137815b3a7abd5db
@hoxyq hoxyq requested a review from fortmarek August 4, 2023 10:31
@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. p: Facebook Partner: Facebook Partner labels Aug 4, 2023
@github-actions
Copy link

github-actions bot commented Aug 4, 2023

Fails
🚫

📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.
Warnings
⚠️ 📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.
⚠️ 📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against ea3efa8

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,490,802 -404,234
android hermes armeabi-v7a 7,808,869 -134,718
android hermes x86 8,960,847 -332,075
android hermes x86_64 8,821,819 -372,622
android jsc arm64-v8a 9,150,105 -331,919
android jsc armeabi-v7a 8,339,578 -83,989
android jsc x86 9,203,167 -262,875
android jsc x86_64 9,461,590 -318,684

Base commit: e22d1a1
Branch: main

@lunaleaps lunaleaps merged commit 03187b6 into facebook:0.72-stable Aug 7, 2023
61 of 63 checks passed
This was referenced Aug 10, 2023
Closed
Merged
@NOUIY NOUIY mentioned this pull request Sep 4, 2023
Open
@cipolleschi cipolleschi mentioned this pull request Sep 21, 2023
Merged
@cipolleschi cipolleschi mentioned this pull request Oct 12, 2023
Merged
@cipolleschi cipolleschi mentioned this pull request Nov 1, 2023
Merged
@cipolleschi cipolleschi mentioned this pull request Dec 27, 2023
Merged
This was referenced Jan 4, 2024
Merged
Merged
Merged
Closed
This was referenced Feb 28, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fortmarek fortmarek Awaiting requested review from fortmarek

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner Pick Request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hoxyq @analysis-bot @lunaleaps @facebook-github-bot