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.

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

Remove forks of TurboModuleBinding/LongLivedObject #11019

Closed
TatianaKapos opened this issue Dec 16, 2022 · 3 comments · Fixed by #12732
Closed

Remove forks of TurboModuleBinding/LongLivedObject #11019

TatianaKapos opened this issue Dec 16, 2022 · 3 comments · Fixed by #12732

Comments

@TatianaKapos
Copy link
Contributor

Problem Description

Recent integration pulling in Remove ObjCTurboModule experiments with retaining JS callbacks in custom scopes removed JS callbacks which we use. Removing the overrides cause JsiSimpleTurboModuleTests to fail. Will need to refactor our code to adjust to new method declarations.

Steps To Reproduce

  1. Remove TurboModuleBinding Overrides located in ReactCommon_TEMP
  2. Try to run IntegrationTests, they will fail

Expected Results

Tests to pass

CLI version

npx react-native --version

Environment

npx react-native info

Target Platform Version

10.0.19041

Target Device(s)

Desktop

Visual Studio Version

Visual Studio 2022

Build Configuration

Debug

Snack, code example, screenshot, or link to a repository

No response

@ghost ghost added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Dec 16, 2022
@TatianaKapos TatianaKapos self-assigned this Dec 19, 2022
@TatianaKapos TatianaKapos added Integration Follow-up Deforking and removed Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) labels Dec 19, 2022
@TatianaKapos TatianaKapos removed their assignment Dec 19, 2022
@TatianaKapos TatianaKapos added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Dec 19, 2022
@marlenecota marlenecota removed the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Dec 19, 2022
@ghost ghost added the Invalid Triage https://github.com/microsoft/react-native-windows/wiki/Triage-Process (label applied by bot) label Dec 19, 2022
@TatianaKapos
Copy link
Contributor Author

@vmoroz I believe you were working on getting these changes to longLivedObjectCollection added back into upstream since we use it in Office code but correct me if I'm wrong! Recent integration brought in JavaTurboModule/LongLivedObject cleanup which caused us to fork more files in ReactCommon. Adding these forked files to this issue so they're on your radar.

@TatianaKapos TatianaKapos changed the title Remove forks of TurboModuleBinding Remove forks of TurboModuleBinding/LongLivedObject Mar 14, 2023
@vmoroz
Copy link
Member

vmoroz commented Mar 14, 2023

@vmoroz I believe you were working on getting these changes to longLivedObjectCollection added back into upstream since we use it in Office code but correct me if I'm wrong! Recent integration brought in JavaTurboModule/LongLivedObject cleanup which caused us to fork more files in ReactCommon. Adding these forked files to this issue so they're on your radar.

Thanks! I still need to go to the RN repo and undo their changes.
BTW, this is a requirement not for Office code. It is to make TurboModules to work if app has more than one RN instance.

@chrisglein chrisglein added this to the Backlog milestone Apr 4, 2023
@chrisglein chrisglein removed the Invalid Triage https://github.com/microsoft/react-native-windows/wiki/Triage-Process (label applied by bot) label Apr 4, 2023
@rozele
Copy link
Collaborator

rozele commented Jan 9, 2024

@vmoroz @TatianaKapos is this what you're looking for: facebook/react-native#42194 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants