Skip to content

feat: make reactHost accessible#48338

Closed
WoLewicki wants to merge 4 commits into
facebook:mainfrom
WoLewicki:@wolewicki/make-react-host-accessible
Closed

feat: make reactHost accessible#48338
WoLewicki wants to merge 4 commits into
facebook:mainfrom
WoLewicki:@wolewicki/make-react-host-accessible

Conversation

@WoLewicki
Copy link
Copy Markdown
Contributor

@WoLewicki WoLewicki commented Dec 19, 2024

Summary:

On bridgeless mode, reactHost is kept in memory even after destroying the DefaultReactNativeHost in brownfield scenario. Since it keeps references to the modules, they are not deallocated, and their initialize methods are not fired again when creating new instance of react-native later. It breaks the behavior of e.g. react-native-screens, which wants to listen for mutations and should get new FabricUIManager: https://github.com/software-mansion/react-native-screens/blob/20b7e83782cd5f79ddd0d61dadc13eeb4db4b258/android/src/main/java/com/swmansion/rnscreens/ScreensModule.kt#L45.

By adding invalidate method, which cleans up the reactHost instance, we can achieve it.

Changelog:

[ANDROID] [FIXED] - add invalidate method for destroying reactHost instance.

Test Plan:

In brownfield scenario, destroy the instance of RN and see that modules are also destroyed.

@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: Software Mansion Partner: Software Mansion Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Dec 19, 2024
@cortinico
Copy link
Copy Markdown
Contributor

/rebase

@cortinico
Copy link
Copy Markdown
Contributor

Thanks for sending this over @WoLewicki This sounds like a reasonable approach, but I'd like to have @mdvacca 's opinion on this as well

@arushikesarwani94
Copy link
Copy Markdown
Contributor

IIRC for internal apps use-case we were able to make use of getReactHost().destroy() to address this scenario without needing an explicit invalidate() in DefaultReactHost

cc: @shwanton

Comment on lines +223 to +224
/** Cleanup function for brownfield scenarios. */
public abstract void invalidate();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that ReactNativeHost already has a clear() method.
Could we instead do the invalidation in the DefaultReactHost.clear() method instead of having to create a brand new invalidate() abstract method?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so clear is a public API for cleaning all of those instances? If so, I think it will be the correct place to put it there then.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it but I am not sure if the INSTANCE is the proper way of getting the instance of kotlin object.

Comment on lines +183 to +185
public fun invalidate() {
reactHost = null
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you instead override fun clear(), call super.clear() and set the reactHost to null here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I get it. DefaultReactHost does not inherit from ReactNativeHost so how can I override clear method in it? Or am I messing it up?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WoLewicki you're right, I misread the class title.

I'd suggest instead we do the following:

  • Here make public fun invalidate() be internal
  • Inside DefaultReactNativeHost add the following:
  override fun clear() {
    super.clear()
    DefaultReactHost.invalidate()
  }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

mReactInstanceManager.invalidate();
mReactInstanceManager = null;
}
DefaultReactHost.INSTANCE.invalidate();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is probably not needed

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@cortinico merged this pull request in 07769d4.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jan 14, 2025
@react-native-bot
Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @WoLewicki in 07769d4

When will my fix make it into a release? | How to file a pick request?

@cortinico
Copy link
Copy Markdown
Contributor

@WoLewicki sorry this took a bit longer. I've also update the commit message/description to reflect the correct behavior 07769d4

@WoLewicki
Copy link
Copy Markdown
Contributor Author

Ok thanks

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

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Software Mansion Partner: Software Mansion Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants