-
Notifications
You must be signed in to change notification settings - Fork 5k
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
[dev-launcher][dev-menu][ios] Add support for Fabric - Part 2 #22821
[dev-launcher][dev-menu][ios] Add support for Fabric - Part 2 #22821
Conversation
b663572
to
7550be1
Compare
@@ -43,15 +43,14 @@ public class ExpoDevLauncherReactDelegateHandler: ExpoReactDelegateHandler, RCTB | |||
ExpoDevMenuReactDelegateHandler.enableAutoSetup = false | |||
|
|||
self.reactDelegate = reactDelegate | |||
self.bridgeDelegate = EXRCTBridgeDelegateInterceptor(bridgeDelegate: bridgeDelegate, interceptor: self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without the EXRCTBridgeDelegateInterceptor
, does the current solution support user's custom delegate in AppDelegate, e.g. if there are custom extraModulesForBridge:
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, let me double check if that still works as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Kudo, you were right about this, the current implementation is not working as expected, do you think it would be ok to pass interceptor
as ExpoDevLauncherBridgeDelegateHandler
?
5487f4f
to
7130a62
Compare
8a780f9
to
1aa28f8
Compare
I'm merging this into #22184 as this PR ended up not being as big as I expected |
ba40903
into
@gabrieldonadel/fabric-ios-dev-menu
Closes ENG-7955 This PR is Part 2 of a series of PRs that will add dev-launcher support for the new architecture. Part 1 #22184 - Fix `EXDevLauncherBridgeDelegate` not able to relaunch DevLauncher - Add `ExpoDevLauncherBridgeDelegateHandler` to handle opening apps from `ExpoDevLauncherReactDelegateHandler` - Update `openDevMenuFromReactNative` to ensure the DevMenu is closed before opening the react-native dev menu Run `fabric-tester` and `bare-expo` on iOS <table> <tr><th>fabric-tester</th><th>bare-expo</th></tr> <tr> <td> <video src="https://github.com/expo/expo/assets/11707729/69e80f31-3d8e-4135-afd7-0f4264052a79"/> </td> <td> <video src="https://github.com/expo/expo/assets/11707729/99b10d00-45b3-4b3e-b7c0-2cc7599ebad7" /> </td> </tr> </table> <!-- Please check the appropriate items below if they apply to your diff. This is required for changes to Expo modules. --> - [ ] Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md). - [ ] Conforms with the [Documentation Writing Style Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md) - [ ] This diff will work correctly for `npx expo prebuild` & EAS Build (eg: updated a module plugin).
Closes ENG-7955 This PR is Part 2 of a series of PRs that will add dev-launcher support for the new architecture. Part 1 #22184 - Fix `EXDevLauncherBridgeDelegate` not able to relaunch DevLauncher - Add `ExpoDevLauncherBridgeDelegateHandler` to handle opening apps from `ExpoDevLauncherReactDelegateHandler` - Update `openDevMenuFromReactNative` to ensure the DevMenu is closed before opening the react-native dev menu Run `fabric-tester` and `bare-expo` on iOS <table> <tr><th>fabric-tester</th><th>bare-expo</th></tr> <tr> <td> <video src="https://github.com/expo/expo/assets/11707729/69e80f31-3d8e-4135-afd7-0f4264052a79"/> </td> <td> <video src="https://github.com/expo/expo/assets/11707729/99b10d00-45b3-4b3e-b7c0-2cc7599ebad7" /> </td> </tr> </table> <!-- Please check the appropriate items below if they apply to your diff. This is required for changes to Expo modules. --> - [ ] Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md). - [ ] Conforms with the [Documentation Writing Style Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md) - [ ] This diff will work correctly for `npx expo prebuild` & EAS Build (eg: updated a module plugin).
Why
Closes ENG-7955
This PR is Part 2 of a series of PRs that will add dev-launcher support for the new architecture.
Part 1 #22184
How
EXDevLauncherBridgeDelegate
not able to relaunch DevLauncherExpoDevLauncherBridgeDelegateHandler
to handle opening apps fromExpoDevLauncherReactDelegateHandler
openDevMenuFromReactNative
to ensure the DevMenu is closed before opening the react-native dev menuTest Plan
Run
fabric-tester
andbare-expo
on iOSScreen.Recording.2023-06-13.at.14.09.29.mov
Screen.Recording.2023-06-12.at.14.43.03.mov
Checklist
npx expo prebuild
& EAS Build (eg: updated a module plugin).