Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47758
Changelog: [internal]
This helps with debugging when adding logs in native code. By default they're logged to stderr and not shown in the Jest output, but we can opt into getting them with this new flag.
Reviewed By: sammy-SC
Differential Revision: D66232572
fbshipit-source-id: d507fa4438ae46988fafc92123d0954ab3d27631
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47673
Update shared monorepo build script:
- Refactor entry point discovery and file iteration
- Add support for "exports" subpath patterns (skip considering as entry point with warning)
This is required for the incoming migration of `react-native-codegen` to this build setup (D51465053).
Changelog: [Internal]
Reviewed By: robhogan
Differential Revision: D66121262
fbshipit-source-id: 0deb2c2b26de442ee348ef9de54a1fa144368b6f
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47685
Currently, we assume any URL with a hostname of `10.0.2.2` or `10.0.3.2` (device-relative) is eligible for rewriting to `localhost` (frontend-relative), because we assume the device is an Android emulator. We rewrite these URLs between device and dev machine so that the rewritten URLs are reachable from the dev machine.
This diff narrows this logic so that we'll only rewrite URLs where the hostname matches the pre-existing list *and* this matches the host the device is actually connected on, according to its headers from the original connection.
The main motivation for this change is to unblock removing assumptions about device-reachable vs server-reachable hosts. Later in the stack we'll drop the hardcoded listing of `10.0.2.2` etc in favour of identifying URLs that target the dev server, from whatever network.
There's also an edge case fix here that `10.0.2.2` etc might actually refer to a remote LAN server, and not be an Android emulator's alias for for an emulator host.
Changelog:
[General][Fixed] RN DevTools: Don't assume 10.0.2.2 is an alias for localhost unless it's used to establish a connection to the server
Reviewed By: huntie
Differential Revision: D66058704
fbshipit-source-id: bad28717b0c9b1ca43e2ea3391cef13f87892e6c
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47719
There is a subset of `ImageProps` that are used by Android but not used by iOS. They were missing in the `ImageProps.h`. It was fine when they were only consumed by the Android mounting layer. But it becomes a problem if we want to write some shared C++ code based on those props.
This diff adds those props to the corresponding C++ type.
This should have no practical effect for both platforms.
Changelog: [Internal]
Reviewed By: javache
Differential Revision: D65426569
fbshipit-source-id: dc1c9fe4a6e0e62e62f84b9b249e1c7d253290f5
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47608
This is a codemod. It was automatically generated and will be landed once it is approved and tests are passing in sandcastle.
You have been added as a reviewer by Sentinel or Butterfly.
Autodiff project: fileops2
Autodiff partition: xplat.js.react-native-github.packages.react-native.ReactCommon.cxxreact
Autodiff bookmark: ad.fileops2.xplat.js.react-native-github.packages.react-native.ReactCommon.cxxreact
This updates `open`, `close`, `read`, `write`, and `pipe` call sites to use
`folly::fileops` qualified name lookup.
This is the 2nd phase in a 3-phase change to remove folly's global definitions
of the posix functions that conflict with windows CRT.
The 1st phase created namespaces for folly's posix functions. The 2nd phase
updates callsites to use the qualified name of folly's `open`, `close`,
`read`, `write`, and `pipe` functions. The 3rd and final phase will remove
folly's globally defined posix functions and have windows CRT define them
again.
**What is the reason for this change?**
Folly's global definitions of posix functions on Windows causes `#include`
order issues if folly is not included first.
For example, when `gtest/gtest.h` is included before folly, gtest includes
`windows.h` and that declares `open`, `read`, and `chdir`, which creates
ambiguous references to folly's `open`, `read`, and `chdir`.
Another example is where posix functions go undeclared when
`folly/portability/windows.h` is included without other portability headers
(e.g., `folly/portability/unistd.h`). `folly/portability/windows.h` includes
`windows.h` in a way that only underscore versions of the posix functions are
available (e.g., `_open`, `_close`).
These issues create friction for windows development.
**Background: What is the purpose of `folly::portability::{fcntl,stdlib,sysstat,unistd}`?**
It is a portability layer to make posix functions available and behave
consistently across platforms. Some posix functions don't exist on windows
(e.g., `sysconf`). Some other posix functions, folly changes to adapt behavior
across platforms. For example, on windows folly defines `open`, `read`,
`write`, and `close` functions to work with sockets. Folly makes these
functions available in the global scope for convenience.
Changelog: [Internal]
Reviewed By: javache
Differential Revision: D65855147
fbshipit-source-id: b06863330ca213b9d1bffe0ee85e0fbf1bc8a845
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47713
`defaultSource` is defined as a single `ImageSource` in docs (https://reactnative.dev/docs/next/image#defaultsource) but its type is a vector of `ImageSource`s in `ImageProps.h`.
This diff changes its C++ type to just `ImageSource`.
Technically, this is a breaking change, however I don't think folks should directly access `ImageProps` outside of `RCTImageComponentView.mm`
Moreover, this prop is actually not implemented in Fabric, so this change should have no practical effect.
Changelog: [Internal]
Facebook
T208171435 - [RN][Fabric][iOS] Implement defaultSource support for Image
Reviewed By: javache
Differential Revision: D65821570
fbshipit-source-id: 38139b0f8d6da495e82c4ef72c19af3db254ba6c
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47637
Changelog: [Internal]
RSNRS does not support off JS thread layout due to current fiber tree corruption when syncing happens on more than one shadow tree at the same time. This diff guarantees that RSNRS will only be enabled on the JS thread, avoiding any state corruption.
Reviewed By: sammy-SC
Differential Revision: D64500893
fbshipit-source-id: aa20f54a0fcfa47534ae099e95307a692bd9fd0f
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47736
This has been 100% enabled in production across all the apps for more than a month, after an experiment a long time ago, and we kinda depend on it now for correct border drawing in some cases. Let's clean it up!
Changelog: [Internal]
Reviewed By: joevilches
Differential Revision: D66208418
fbshipit-source-id: 582a38bd84a2d085ba5c4aac4cd478680dd206cc
Summary:
X-link: https://github.com/facebook/yoga/pull/1746
Chrome made some changes for how overflowed row-reverse containers are laid out which was causing some issues on CI. I updated them here and skipped the new failing tests which we would want to followup on.
For LTR, the differences are seen below
|Before|After|
|--|
|{F1962694149} | {F1962694151}|
The extra space is now extending past the flex start edge vs flex end. RTL is the opposite. NickGerleman had deviated from the spec back in the day to match Chrome and it seems they made the adjustment recently. T208209388 is tracking the followup to align with the spec again. Basically, there is a notion of fallback alignment when certain justification/alignment values cannot actually apply. Right now we are falling back to flex start in all cases but we should fallback to start sometimes.
Changelog: [Internal]
Reviewed By: NickGerleman
Differential Revision: D66138361
fbshipit-source-id: c46d2e9b0cd297069b9cc544e3bded995e4867a6
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47689
We were seeing some segfaults from trying to access `display_` in Yoga's `replaceChild` function. Some memory debugging helped identify this was a use-after-free error and NickGerleman suggested we try swapping these lines. Logic being the shared_ptr of YogaLayoutableShadowNode is replaced right before we go and replace its yoga node in the yoga tree. If this is the last shared_ptr holding this node we will delete this object and thus the yoga node with it. We do not need to do this first, so let's swap the lines.
Changelog: [Internal]
Reviewed By: NickGerleman
Differential Revision: D66142356
fbshipit-source-id: 8fd835346edc91e045ed2ee8945a95af21c47556
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47618
Its not optimal to reconstruct CompositeBackgroundDrawable everytime we add a layer to it.
With this change I'm adding an optimization to modify the underlying `LayerDrawable` in place instead of reconstructing everything. `LayerDrawable` has a pretty constrained API and some weirdish behaviors.
- `addLayer` - This API doesnt set the callback for the recently added layer so after adding the layer we also need to manually set the callback
- Mutating `LayerDrawable` in-place is not straightforward, I had to add a function that will figure out where each layer should be inserted
- `LayerDrawable` doesn't allow deleting an element which means that for this case in particular we do need to re-create `CompositeBackgroundDrawable`
- Newer Android versions allow `null` on `LayerDrawable` layers, but older versions do not, this implementation is mostly done this way to accommodate older versions. But also, even though newer versions can have `null` set on a layer `LayerDrawable` still doesn't handle it well and we get some bugs when removing and inserting a layer which the current implementation handles by not relying on null
This is all feature flagged. since it will only be enabled with the new drawables
Changelog: [Internal]
Reviewed By: joevilches
Differential Revision: D65907786
fbshipit-source-id: c18b898ddfe58faa5b90714945ffcc82d471051d
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/46818
Before we were using lists which meant `CompositeBackgroundDrawable` had a `LayerDrawable` of variable length.
This meant that our layers were not consistent and made it hard to optimize `CompositeBackgroundDrawable` to not create a new instance every time we set a new Layer.
With this change `CompositeBackgroundDrawable` has a consistent amount of layers which will allow us to more easily mutate it and land the optimization present on D63287222
Changelog: [Internal]
Reviewed By: NickGerleman
Differential Revision: D63798842
fbshipit-source-id: bf0b9ee91ff2083b89ca90ace4de00c71bf3153c
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47710
There is Android code that supports remapping `defaultSrc` -> `defaultSource` for `ImageNativeComponent`.
The docs reference this prop as `defaultSource`: https://reactnative.dev/docs/next/image#defaultsource
It is not referenced as `defaultSrc` anywhere. Let's unify it as `defaultSource` across both platforms.
Changelog: [Internal]
Reviewed By: javache
Differential Revision: D65819218
fbshipit-source-id: 0f468e2327ad07285a45e4c9f5e33d74da411c74
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47676
`getModule` is used by all modern ReactPackage base-classes to allow lazy loading of specific module and should be considered stable at this point.
Changelog: [Android][Added] Marked ReactPackage#getModule as stable.
Reviewed By: mdvacca
Differential Revision: D66127068
fbshipit-source-id: de447794435d267619430e3c0cbc65c6e1c614ba
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47669
When investigating [#47476](https://github.com/facebook/react-native/issues/47476), I found that the `secureTextInput` prop was not changing in the Mounting layer when changing it in JS.
I track down the problem to the `UIManager::cloneNode` method.
When we clone the node, we first merge the patch that arrives from React into the props controlled by setNativeProps, ignoring the patch's props that are controlled by React.
But then, we forgot to merge back the React's controlled property into the final props, effectively losing them.
This change adds an extra merging step, merging the props controlled with setNativeProps back into the patch of props controlled by React, and then using this new set of props as source of truth.
## Changelog:
[General][Fixed] - do not discard props in the patch when they are not null while using `useNativeProps`
Reviewed By: sammy-SC
Differential Revision: D65948574
fbshipit-source-id: db4f2b793f4a6348456933c95a151012252b8ebc
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47704
Now that we have a min requirement of Node 18.18 on `main`, drop dependency on this polyfill.
Changelog: [Internal]
Reviewed By: vzaidman
Differential Revision: D66162328
fbshipit-source-id: e8ab6669fe14ed177eccf4b861c01df4fb0d405a
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47718
We have a test in the OSS E2E CI pipeline that is a bit flaky and fails often on Android.
I don't have time to investigate that properly right now, so I'm disabling it to improve the situation on the main branch for the time being.
We are planning to invest more resources in H1 2025 to improve the E2E testing in OSS, so I'll get back to it soon.
## Changelog
[Internal] - Disabling part of the FlatList E2E test in Maestro
Reviewed By: javache
Differential Revision: D66169183
fbshipit-source-id: 5b2b0c45e124a642b626b014b91fa61d17226f9b
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47700
Changelog: [internal]
Add support for the string parameter for `toThrow` to assert for specific error messages.
Reviewed By: sammy-SC
Differential Revision: D66118001
fbshipit-source-id: 8c04cd20d4ad17163ec0c7bf943c429507a97985
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47699
Changelog: [internal]
Implements a `jest.fn()` and a subset of methods in `expect` using them (`.toHaveBeenCalled()` and `.toHaveBeenCalledTimes()`).
Reviewed By: sammy-SC
Differential Revision: D66118002
fbshipit-source-id: 5422307d68967d7d8b4c4d5155a45926f8fc8cae
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47706
changelog: [internal]
The changelog in D65909191 already has the details. The fix is behind a feature flag. Let's enable it in OSS to stop the bleeding.
Reviewed By: cipolleschi
Differential Revision: D66163663
fbshipit-source-id: 84a3dfb823fdd286e919ebf22b07054b4399662a
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47678
No longer referenced and removed from public exports in D49752133
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D66127071
fbshipit-source-id: 90284b26051902243b530658e5795dad31331695
Summary:
In its current for the `podspec_source_build_from_local_source_dir` makes an assumption in the existence of the `artifacts_dir` ("Pods/hermes-engine-artifacts"). This is okay in the case where the `pod install` command is first ran without sitting the `REACT_NATIVE_OVERRIDE_HERMES_DIR` because of
1948076b81/packages/react-native/sdks/hermes-engine/hermes-utils.rb (L227)
In a clean checkout however, this results in an error when archiving the Hermes directory due to the missing parent directory of the archive's destination.
This PR suggests adding a check for and optional creation of the existence of the archive directory before usage.
An alternative that I considered was ensuring the existence inside of the `artifacts_dir` function:
1948076b81/packages/react-native/sdks/hermes-engine/hermes-utils.rb (L198-L200)
But there's only one other call site, which already does the `mkdir -p`.
## Changelog:
<!-- Help reviewers and the release process by writing your own changelog entry.
Pick one each for the category and type tags:
[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message
For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->
[IOS] [FIXED] - Ensure artifacts directory exists when building Hermes from local source
Pull Request resolved: https://github.com/facebook/react-native/pull/47698
Test Plan:
I verified the fix by:
- Doing a local git clone of the `hermes` repository.
- Ensure no `Pods` directory exists.
- `export REACT_NATIVE_OVERRIDE_HERMES_DIR=/your/local/path/to/hermes` pointing to the local `hermes` repository.
- Run `pod install`
Reviewed By: cipolleschi
Differential Revision: D66162175
Pulled By: dmytrorykun
fbshipit-source-id: 322633e217063e7ca199b9a9602e279df5fbdb70
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47670
Now that we have `babel-plugin-syntax-hermes-parser` in `react-native/babel-preset` (since D63535216), it's no longer necessary to use `hermes-parser` directly in Metro in order to use newer Flow syntax.
Babel with `babel-plugin-syntax-hermes-parser` is generally preferable, because it intelligently falls back to parsing with Babel for any non-`flow` files.
See https://github.com/facebook/hermes/issues/1549 for context.
Changelog:
[General][Fixed] metro-config: Don't use `hermes-parser` by default, prefer `babel-plugin-syntax-hermes-parser`, which supports other syntax plugins.
Reviewed By: huntie
Differential Revision: D66002056
fbshipit-source-id: cf48acec347e2c0791872f8ca4b53f5f8af1c783
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47690
Creates a feature flag to evalute the impact of disabling `InteractionManager` in `Batchinator` and synchronously invoking callbacks after the timeout.
This also deletes the `dispose` arguments in `Batchinator` that were unused.
Changelog:
[Internal]
Reviewed By: bvanderhoof
Differential Revision: D66139643
fbshipit-source-id: d17bab0cd25c0c69779686cb435c063f707255e4
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47684
jsBundleLoader parameter was added into DefaultReactHost.getDefaultReactHost but we decided to revert this change.
We want to keep DefaultReactHost.getDefaultReactHost as simple as possible and we don't want to include this change in 0.77
changelog: [Android][Breaking] Remove jsBundleLoader from DefaultReactHost.getDefaultReactHost()
Reviewed By: shwanton
Differential Revision: D66131197
fbshipit-source-id: 7451bb55d7953d3282b23d23ad15e91bae71ff24
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47598
## Changes
Now:
- in bridgeless, NativeModules.foo will also return turbo modules. And, global.__turboModuleProxy no longer exists.
- in bridge, nothing changes.
| **JS API** | **Bridge** | ***[Before]* Bridgeless** | ***[Before]* Bridgeless w/ Interop**| ***[After]* Bridgeless**
| global.__turboModuleProxy | turbo modules | turbo modules | turbo modules |**deleted**
| global.nativeModuleProxy | legacy modules | error | legacy modules | turbo + legacy modules
## Justification
This reduces the cost for adopting the new architecture:
- Prior, you had to migrate the module itself, **and** all its callsites: NativeModules.foo -> NativeFoo
- Now, you have to migrate the module itself **only**.
This simplifies the interop layer logic in bridgeless: all modules come from the same thing.
Changelog: [General][Breaking] Bridgeless: Make NativeModules.foo load turbomodules (unset turboModuleProxy in bridgeless).
Reviewed By: javache
Differential Revision: D65896934
fbshipit-source-id: 10883c292b78759fceac5bd984e0cdf8a679fc67
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47653
## Context
Currently, when `nativeSourceCodeFetching == false`, `inspector-proxy` attempts to pre-fetch source maps, given the URL from a `Debugger.scriptParsed` event, and embeds them into `Debugger.scriptParsed`'s `sourceMapURL` using a data URI.
This was originally to support frontends that did not perform HTTP requests or were blocked (eg by CORS), but we're retaining it for the moment because it's more performant than lazy loading the source map.
Similarly, we perform middleware->server fetches to respond to `Debugger.getScriptSource` events.
To make these fetches for URLs that target `10.0.2.2` (ie, addressable from within an Android emulator) (etc), we rewrite `10.0.2.2`->`localhost` and perform a `fetch` from the Node process running dev-middleware.
## The problem
Consider a setup where:
- Metro is running on a remote server, listening on `8081`.
- Dev machine tunnels `localhost:8082` -> remote `8081`.
- An app is running on an Android emulator on the dev machine, with bundle URL configured to `10.0.2.2:8082`.
In this case, we'll rewrite `10.0.2.2:8082` to `localhost:8082`, which *is* reachable and correct from the dev machine, but *not* from the machine where Metro is running, so the `fetch` of a source map from the inspector proxy will fail.
## Motivation
This might seem like a niche case, but it's part of fixing a series of unsafe assumptions that currently prevent us from running DevTools on an arbitrary port.
## This fix
Preserve the current behaviour (simple `10.0.2.2`<=>`localhost`) for URLs sent to the frontend, but construct a separate, server-relative URL, using the configured `serverBaseUrl`, for `fetch` calls within dev-middleware.
Changelog:
[General][Fixed] RN DevTools: Fix fetching sources and source maps when the dev-server is remote and not tunnelled via the same port+protocol.
Reviewed By: huntie
Differential Revision: D65993910
fbshipit-source-id: a0cdcf1644e97a2af3d8583f2da2aaa51276f68c
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47675
Use `request` over `fetch` in `dev-middleware`'s tests.
This is required by the next diff in the stack to spoof the `Host` header for testing purposes, which isn't permitted by the `fetch` spec.
The return type is a bit different (eg `statusCode` vs `status`, no `ok` prop), but the modifications needed are pretty straightforward.
Changelog: [Internal]
Reviewed By: huntie
Differential Revision: D66005427
fbshipit-source-id: f311b0188d6d0ec220a037774fca78df5373163a
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47628
`serverBaseUrl` is currently documented as:
> The base URL to the dev server, as addressible from the local developer machine
This is problematic in general because `dev-middleware` on a server doesn't necessarily know about where clients might be reaching it from, how tunnels or port-forwards are set up, etc., and this can change over the lifetime of the server and vary between clients.
Indeed, our own use of `serverBaseUrl` from both `community-cli-plugin` and internally simply sets it to the host and port the dev server is listening on - ie it's the address of the dev server accessible *from the server*.
This PR changes the docs, redefining `serverBaseUrl`, to match the way we currently specify it.
One usage where we *do* want the previously documented behaviour is in responses to `/json/list` (`getPageDescriptions`) where the URLs in the response should be reachable by a browser requesting `/json/list`.
Here, we use the request (host header, etc.) to attempt to get working base URL.
History:
It should be mentioned that this is the latest in a series of changes like this:
- https://github.com/facebook/react-native/pull/39394
- https://github.com/facebook/react-native/pull/39456
Learning from those:
- This change does *not* break Android emulators, which routes `10.0.2.2` to localhost, or other routed devices, because `/open-debugger` still uses server-relative URLs, and now formally delegates to `BrowserLauncher` to decide what to do with those URLs (internally, VSCode / `xdg-open` handles port forwarding)
- Middleware configuration is no longer required to specify how it is reachable from clients.
This sets up some subsequent changes for more robust handling of tunnelled connections.
Changelog:
[General][Breaking] dev-middleware: Frameworks should specify `serverBaseUrl` relative to the middleware host.
Reviewed By: huntie
Differential Revision: D65974487
fbshipit-source-id: 1face8fc7715df387f75b329e80932d8543ee419
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47652
## Background
When the `nativeSourceCodeFetching` capability is disabled, `inspector-proxy` rewrites URLs exchanged over CDP between device and frontend so that URLs are addressable from CDT - in particular, when using an Android emulator `10.0.2.2` (host's address from within the emulator) is rewritten to and from `localhost` (the equivalent address reachable from the host).
Previously - before we implemented `Network.loadNetworkResource`, or on old frontends that don't attempt to use that method - this worked reasonably well. A `fetch` from CDT to Metro would succeed on the rewritten URL.
## Problem
Since we implemented `Network.loadNetworkResource`, but disabled the `nativeSourceCodeFetching` capability, source fetching is broken under Android emulators. We're rewriting URLs to be frontend-relative, but then attempting to fetch them through the device, because as far as CDT is aware, `Network.loadNetworkResource` should still be tried first.
When `Network.loadNetworkResource` responds with a CDP *error*, CDT falls back to a local fetch (which would work), but when it responds with a CDP *result* of `success: false`, there is no fallback.
## Fix
This diff adds an interception guarded behind `nativeSourceCodeFetching == false`, which rejects any calls to `Network.loadNetworkResource` with a CDP error. This restores the previous behaviour from before `Network.loadNetworkResource` was implemented at all.
NOTE: An alternative approach would be to rewrite URLs back to device-relative for `Network.loadNetworkResource`, but IMO it's more correct for the frontend to respect that the device is asserting that it doesn't have that capability, and not to try to use it.
Changelog:
[Android][Fixed] RN DevTools: Fix source loading when using an Android emulator connecting to a dev server on the host.
Reviewed By: huntie
Differential Revision: D66074731
fbshipit-source-id: f2050c014cd5cfa546bff5e9d0412413a5daff35
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47623
Changelog:
[iOS][Fixed] - "Reconnect DevTools" button not working sometimes
Hotfix for "Reconnect DevTools" button not working:
* T206141946 / [WP: Reconnecting dev tools does not work after restarting the app](https://fb.workplace.com/groups/rn.debugger.feedback/posts/1107620434125533)
* T206754760 / [WP: Can't launch DevTools from Metro sometimes](https://fb.workplace.com/groups/rn.debugger.feedback/posts/1112235073664069/)
Basically, this dialog will keep reappearing like a bad dream:
{F1960030622}
# Repro
Part 1 (Fixed in base stack D65973309)
1. Do NOT have Metro ready.
1. Build and run RNTester/FB Wilde
1. They should be using the local bundled version. App may prompt you to start Metro.
1. Start Metro
1. Go to the device Dev Menu (rage shake) and select Reload
1. Press `r` or `d` in Metro
Expected: Reload and Dev Menu work accordingly
Actual: Metro fails with `No apps connected`:
{F1960039703}
Part 2 (Fixed in this diff)
1. Open React Native DevTools via Metro `j` key or Dev Menu (rage shake)
1. Kill Metro
1. The RN DevTools should show the "disconnected" dialog
1. Start Metro
1. Click "Reconnect DevTools" in RN DevTools
Expected: reconnects
Actual: dialog reappears with an error in Metro:
{F1960043097}
Interestingly, the `r` and `d` keys from Metro works.
# Root cause(s)
Part 1: See D65973309
Part 2:
The error indicates the target/device failed to call `/inspector/device` to register itself. The subsequent calls to `/json/list` returns empty and `/open-debugger` throws.
1. But because `r` & `d` (heh) works, we can observe that there is some kind of auto-reconnect mechanism:
https://www.internalfb.com/code/fbsource/[cfe1706a60b2]/xplat/js/react-native-github/packages/react-native/Libraries/WebSocket/RCTReconnectingWebSocket.m?lines=77-82
1. We do have auto-reconnect for `j` too:
https://www.internalfb.com/code/fbsource/[cfe1706a60b2]/xplat/js/react-native-github/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnection.cpp?lines=246-254
But unfortunately it only tries once. A long-term fix would be calling reconnect recursively like the Objective-C impl above, e.g.
Edit: the long-term fix ! See V3
```
delegate_->scheduleCallback(
[weakSelf = weak_from_this()] {
auto strongSelf = weakSelf.lock();
if (strongSelf && !strongSelf->closed_) {
strongSelf->reconnectPending_ = false;
strongSelf->connect();
// Keep trying. Never. Give. Up.
if (!strongSelf->isConnected()) {
strongSelf->reconnect();
}
}
},
RECONNECT_DELAY);
```
Edit: I snuck in some time during React Native London Conf and got the long-term fix going! ✌️
{gif:7iyrns4l}
~~It appears that the current impl of `isConnected()` is not a true reflection of the web socket state. My time box for this task ran out, so we'll do a hot fix for the short-term: since we know `r` & `d` reliably reconnects, we'll piggy-back on its lifecycle to attempt reconnection. This works. I'm going on PTO for the year, so the follow-up task is up for grabs here: T207775935.~~
# PS
1. If you start the app with Metro running in step 1, this bug is not present. This is the reason why FB Wilde/Marketplace/Quantum engineers run into this more often (because its custom menu changes the JS URL after start up)
2. This auto-reconnect does not mean the RN DevTools frontend will dismiss the dialog automatically. This is only about the `Metro <> Device` in `Frontend <> Metro <> Device`.
1. Current impl in Metro/Inspector Proxy means that whenever the `Metro <> Device` connection is terminated, the `Frontend <> Metro` connection is killed.
1. This diff helps restore the `Metro <> Device` connection.
1. The `Frontend <> Metro` connection does not currently get reconnected. We can make the Frontend do so, with consideration of preserving logs/states, etc.
Reviewed By: robhogan
Differential Revision: D65952134
fbshipit-source-id: 3abd16f6a7b7ed50e8acdb7c753cc4fbd3317236
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/47651
## This Change:
This change uses in the App's AppDelegate the newly generated `RCTAppDependencyProvider`, passing it to the `RCTAppDelegate`.
This change needs to be applied also to the template, when this stack lands.
## Context
React Native has a last temporal dependency on Codegen in the React-RCTAppDelegate pod.
The RCTAppDelegate has the responsibility to provide various dependencies to react native, like third party components and various modules. ReactCodegen is generated when the user create the project, while React-RCTAppDelegate eists in React Native itself.
This dependency means that we cannot prepare prebuilt for iOS for React Native because when we would have to create prebuilds, we would need the React Codegen, but we can't create a React codegen package that will fit all the apps, because React Codegen can contains App Specific modules and components and apps might have different dependencies.
## Changelog:
[iOS][Added] - Pass the `RCTAppDependencyProvider` to the `RCTAppDelegate`
Reviewed By: dmytrorykun
Differential Revision: D66074475
fbshipit-source-id: 93bf500fe37f115352ebd49d3d56955cbaeeea72
