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

chore(Android): migrate MessageQueueThread and it's implementation to Kotlin #48652

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

TheRogue76
Copy link
Contributor

Summary:

Continuing our usual journey, this time migrating MessageQueueThread. Was not expecting to see that many assertions in ReactContext.
One important thing to note on this PR: I had to add an extra Throw RuntimeException to startNewBackgroundThread. It already had one from thedataFuture.getOrThrow(), but if your thread is not associated with a Looper, calling myLooper (Line 203 of MessageQueueThreadImpl) Can return null. Until now, this would have been a NPE, i just decided to make it a RuntimeException with the message Looper not found for thread. Let me know if you want me to make that function return a nullable, or throw another message or exception, or if you want me to treat Looper as Nullable in the whole class.

Changelog:

[INTERNAL] [FIXED] - Migrate MessageQueueThread and MessageQueueThreadImpl to Kotlin

Test Plan:

Screenshot 2025-01-13 at 21 03 00

@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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jan 13, 2025
bgThread.start()

val pair = dataFuture.getOrThrow()
val looper = pair?.first ?: throw RuntimeException("Looper not found for thread")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would love to know if this is ok, or another approach should be taken

@facebook-github-bot
Copy link
Contributor

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

@TheRogue76
Copy link
Contributor Author

@rshest Can i know what the test that failed was doing?

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. 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.

2 participants